NO-JIRA: Fix iterating over stream.Status.Tags#2307
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vincentdephily: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesImage Stream Graph Updates
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/lgtm We will need to verify this manually, I guess. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@tchap Retriggered the AI review, if that what you alluded to with "verified manually". Though IMHO the AI missed the main point of the fix : the old code used to |
I mean that there is no unit test, so pls label it as verified when you test it manually. |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
I'm not sure what is the main motivation of this PR. Can you please post an example of previous log and current log?
fc158ac to
9fa3021
Compare
|
New changes are detected. LGTM label has been removed. |
It's an `[]T`, not a `map[string]T`, tag string is in `T.Tag`. Only affects log output. Mistake was AFAICT here from the start, but only showed up a typecheck error with go 1.26.
9fa3021 to
d5b1306
Compare
The initial motivation is a miscompilation with go 1.26 (an over-eager attempt, I won't actually be going there), which warns about formatting integers with I'm spinning a cluster up to check the output, though I'm not sure of how to hit that specific usecase. I expect a log with |
| if imageNode == nil { | ||
| klog.V(2).Infof("Unable to find image %q in graph (from tag=%q, dockerImageReference=%s)", | ||
| history.Items[i].Image, tag, image.DockerImageReference) | ||
| image.Image, history.Tag, image.DockerImageReference) |
There was a problem hiding this comment.
Does content of history.Tag differ from tag?
There was a problem hiding this comment.
- The original iteration was
for tag, history := range stream.Status.Tags stream.Status.Tagsis a[]imagev1.NamedTagEventList- So
tagin the original code is an integer, the position in the array - The original code probably expected to be looping over a map with the tag as keys and
NamedTagEventListas values - Blame Go iteration API for the footgun. Claude kept hallucinating the same way as the original code.
I wasn't sure how to get into a state where Original: Patched: If you prefer, I could probably augment |
| for i := range history.Items { | ||
| image := history.Items[i] | ||
| for _, history := range stream.Status.Tags { | ||
| for i, image := range history.Items { |
There was a problem hiding this comment.
edgeKind := ImageStreamImageEdgeKind
if i > 0 {
edgeKind = HistoricImageStreamImageEdgeKind
}
g.AddEdge(imageStreamNode, imageNode, edgeKind)
I suppose there are other ways to write this, but I just wanted a minimal patch, not touching the logic.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, tchap, vincentdephily The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retest |
|
CI looks resource-starved. |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
@vincentdephily: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Iteration is over an
[]T, not amap[string]T, tag string is inT.Tag.Only affects log output. Mistake was AFAICT here from the start, but only showed up a typecheck error with go 1.26.
Summary by CodeRabbit