oc logs must wait for first build from bc with ConfigChange trigger #12163
Conversation
| // Wait until timeout before giving up on the first build | ||
| return false, nil | ||
| } else { | ||
| return false, fmt.Errorf("no builds found for %q", t.Name) |
There was a problem hiding this comment.
isn't this error going to get wrapped/eaten by line 495? I think we want to retain the existing output behavior for cases where there was no build found and no configchangetrigger on the BC.
There was a problem hiding this comment.
Why are we waiting in this code? This code should not be waiting for anything. If you need to wait, do it on the server.
There was a problem hiding this comment.
there is no buildconfig/logs api endpoint that could do the waiting, we're just writing a clever client here.
setting up a watch on builds (which would still amount to the client waiting) seems like overkill for this trivial fix.
There was a problem hiding this comment.
If this is something that a naive client would want to do, then we should be fixing this on the server. I am not in favor of overly complex code here to fix one client.
There was a problem hiding this comment.
if that is true then the entire "logsforobject" concept is flawed and should have been done as api endpoints, no?
There was a problem hiding this comment.
@bparees I've given the potential outcomes their own error messages in the latest push.
| return nil, fmt.Errorf("no builds found for %q", t.Name) | ||
|
|
||
| hasConfigChangeTrigger := false | ||
| if t.Spec.Triggers != nil && len(t.Spec.Triggers) > 0 { |
There was a problem hiding this comment.
Should be an utility HasConfigChangeTrigger or something.
There was a problem hiding this comment.
Added utility method in buildapi.
| } | ||
|
|
||
| err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { | ||
| builds.Items = buildapi.FilterBuilds(builds.Items, buildapi.ByBuildConfigPredicate(t.Name)) |
There was a problem hiding this comment.
You are never updating this list of builds meaning it's going to always be the same, right?
There was a problem hiding this comment.
Thanks @Kargakis . Fixed in next push.
| builds.Items = buildapi.FilterBuilds(builds.Items, buildapi.ByBuildConfigPredicate(t.Name)) | ||
| if len(builds.Items) > 0 { | ||
| return true, nil | ||
| } else { |
| if hasConfigChangeTrigger { | ||
| // Wait until timeout before giving up on the first build | ||
| return false, nil | ||
| } else { |
There was a problem hiding this comment.
Using more compact style in next push.
| err = nil | ||
| } | ||
| } | ||
| return buildTriggers, err |
There was a problem hiding this comment.
Seems weird to return an error for this. i would expect the caller to check the array length and if it's zero, they know it wasn't found. errors should be used for issues that are harder to anticipate imho.
There was a problem hiding this comment.
I prefer your methodology. My initial pass was to preserve/unify on the signature I found in webhooks. Will make the change.
| hasConfigChangeTrigger := buildapi.HasTriggerType(buildapi.ConfigChangeBuildTriggerType, t) | ||
|
|
||
| var builds *buildapi.BuildList | ||
| err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { |
There was a problem hiding this comment.
I think I annoyed @smarterclayton enough that he decided to let this slide for now, but it does sound like if we find ourselves in this code much more, we should consider introducing a server side build/bc logs api endpoint.
|
lgtm [merge] |
|
Evaluated for origin merge up to cd418ff |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to cd418ff |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12214/) (Base Commit: a954b6c) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12230/) (Base Commit: de5ab20) (Image: devenv-rhel7_5519) |
Fixes #12142
ptal @bparees