Implement logs streaming option when starting a build#1197
Implement logs streaming option when starting a build#1197openshift-bot merged 1 commit intoopenshift:masterfrom 0xmichalis:stream-logs
Conversation
pkg/cmd/cli/cmd/startbuild.go
Outdated
There was a problem hiding this comment.
do we only support streaming build-logs for a single build at a time, for a given buildconfig?
There was a problem hiding this comment.
@bparees yes, if you take a look at the code behind osc build-logs, it's the same as the methods I have invoked here ie. specify a build or a buildConfig and stream its logs.
There was a problem hiding this comment.
there shouldn't be, which is why i'm so confused..your pastebin implied this was working, but i'm not sure how :)
the terminology is a bit wonky, but you use a buildconfig to create a build, so if you start the same buildconfig twice, you'll get two unique build objects created (note the distinction of buildconfig vs build), each with their own pod, own logs, etc.
There was a problem hiding this comment.
I am familiar with the distinction between builds and buildconfigs, just getting around them. Since every build will be in its own pod i guess you will see the same logs twice. I will test it tomorrow.
There was a problem hiding this comment.
@bparees now that it's morning here and I can function normally: the reason the pastebin I posted seems like working is because when running with --from-build specified, it's essentially re-running a previous build so buildName is the name of a build in that particular case.
There was a problem hiding this comment.
note: By the time I redirect buildLogs as it is right now, there is no newBuild.Name set.
There was a problem hiding this comment.
note: By the time I redirect buildLogs as it is right now, there is no newBuild.Name set.
Which means that this code is incorrect as is. If I am going this way, I should kick off the goroutine after I have the newBuild created so I can access its name.
0xmichalis
changed the title
|
Not sure if I am on the right track here, though these latest changes seem much more correct than the initial code of this pr. |
pkg/cmd/cli/cmd/startbuild.go
Outdated
There was a problem hiding this comment.
why can't you just do this before you start the follow logic, rather than using a channel to send newBuild.Name in later?
There was a problem hiding this comment.
I was thinking that the watcher would be already setup watching for builds by the time we started the build but in the end I think you are right. I 'll investigate it more and probably change to what you suggest.
|
After the latest Kube rebase: 🎉 http://pastebin.com/J56uFUZJ 🎉 http://pastebin.com/xeQdWs3a 🎉 |
|
lgtm...squash and we can merge after you confirm it's working post-rebase. |
0xmichalis
changed the title
|
LGTM |
|
tested. passed. [merge]d. |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1123/) (Image: devenv-fedora_982) |
|
Evaluated for origin up to 50f134e |
6 participants
(cc: @bparees @jhadvig @mfojtik )
Closes #645