-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to start-build --follow, somewhat contradicting #6268 #11119
Conversation
edf12d2
to
199e700
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit. I don't think we need new tests for this, we should already have ample coverage of this space and i'm not worried about a specific test for the retry logic.
if err != nil { | ||
// retry the connection to build logs when we hit the timeout. | ||
if oerrors.IsTimeoutErr(err) { | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should print a message to the user each time we timeout and start waiting again, so they know we're still waiting for the build to start/logs to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bparees, done, ready to go if you're happy with it.
- make --follow always retry on timeouts - make --follow imply --wait: guarantee returning an error, or ensure stream has finished and build status is in complete, failed, cancelled, error (aka not new, pending, running) - remove possible undesirable race condition (I think --follow --wait could unwittingly truncate returned logs)
199e700
to
335e1c3
Compare
lgtm |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to 335e1c3 |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9359/) |
looks like it failed due to a network glitch - couldn't connect to github.com:443 |
flake #11016 [merge] |
Evaluated for origin merge up to 335e1c3 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9467/) (Image: devenv-rhel7_5099) |
has finished and build status is in complete, failed, cancelled,
error (aka not new, pending, running)
could unwittingly truncate returned logs)