tolerate multiple bcs pointing to same istag for oc status #9308
Conversation
| istNode := uncastIstNode.(*imagegraph.ImageStreamTagNode) | ||
| if bcNode := buildedges.BuildConfigForTag(g, uncastIstNode); bcNode != nil && !istNode.Found() { | ||
| // findPendingTags is the guts behind FindPendingTags .... break out some of the contents and reduce some indentation | ||
| func findPendingTags(bcNodes []*buildgraph.BuildConfigNode, markers []osgraph.Marker, istNode *imagegraph.ImageStreamTagNode, uncastIstNode graph.Node, g osgraph.Graph, f osgraph.Namer) []osgraph.Marker { |
There was a problem hiding this comment.
The number of arguments here smells bad ... almost not sure if we should be passing in istNode and bcNodes if they're trivial to get from the uncastIstNode.
Similarly, we don't need the markers, we can just return our own list and FindPendingTags can do the append.
There was a problem hiding this comment.
refactored ... part of just completed push
|
updates based on comments from @stevekuznetsov pushed - thanks. |
| @@ -162,6 +162,62 @@ func FindCircularBuilds(g osgraph.Graph, f osgraph.Namer) []osgraph.Marker { | |||
| return markers | |||
| } | |||
|
|
|||
| // findPendingTags is the guts behind FindPendingTags .... break out some of the content and reduce some indentation | |||
| func findPendingTags(uncastIstNode graph.Node, g osgraph.Graph, f osgraph.Namer) []osgraph.Marker { | |||
There was a problem hiding this comment.
There is only one tag and you are returning markers: findPendingTagMarkers ?
There was a problem hiding this comment.
part of next push
|
I like the refactor, much easier to read. |
|
updates based on comments pushed ... thanks @deads2k @Kargakis @stevekuznetsov |
|
|
||
| Severity: osgraph.WarningSeverity, | ||
| Key: TagNotAvailableWarning, | ||
| Message: fmt.Sprintf("%s needs to be imported or created by a build.", f.ResourceName(istNode)), |
There was a problem hiding this comment.
We may still want to show this message in case a build failed but I don't feel strong about it. We already point to the build failure and deployment config markers take care of the deployment part.
|
LGTM but I will defer to @deads2k and @stevekuznetsov for a final review |
| func findPendingTagMarkers(istNode *imagegraph.ImageStreamTagNode, g osgraph.Graph, f osgraph.Namer) []osgraph.Marker { | ||
| markers := []osgraph.Marker{} | ||
|
|
||
| var latestBuild *buildgraph.BuildNode |
There was a problem hiding this comment.
buildFound := false? The actual value isn't important, right?
There was a problem hiding this comment.
correct ... part of next push
|
One nit. lgtm |
| // use of f.ResourceName(bcNode) will produce a string like oc start-build BuildConfig|example/ruby-hello-world | ||
| ret = ret + fmt.Sprintf("oc start-build %s", bcNode.BuildConfig.GetName()) | ||
| if i < (len(bcNodes) - 1) { | ||
| ret = ret + ", " |
There was a problem hiding this comment.
This makes the line not copy-paste-able into the shell... do we care? Do we want each suggestion on it's own line?
There was a problem hiding this comment.
This makes the line not copy-paste-able into the shell... do we care? Do we want each suggestion on it's own line?
I think this is better. Otherwise I'll get tons of lines.
There was a problem hiding this comment.
Copy-paste is your horse to beat, so that sounds good :)
|
output nit, otherwise LGTM |
|
[merge] |
|
ah - thanks @deads2k (didn't realize you had that power) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4905/) (Image: devenv-rhel7_4382) |
|
Evaluated for origin merge up to de1519d |
|
@deads2k comes in here like |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to de1519d |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4905/) |
Fixes #8022
@Kargakis @deads2k @stevekuznetsov @csrwng PTAL if you have the cycles