clusterimageset-updater: use multi payload for 4.12+, amd64 for <4.12#4975
clusterimageset-updater: use multi payload for 4.12+, amd64 for <4.12#4975deepsm007 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (2)
WalkthroughThe changes add architecture determination logic to support multi-architecture payloads for OpenShift versions 4.12 and later. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
|
/lgtm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/clusterimageset-updater/main_test.go (1)
26-33: Add a major-version boundary case to lock behavior.Current cases are strong for
4.x, but there’s no guard for5.x(or other non-4.x) parsing behavior. Adding that case will prevent silent regressions inarchitectureForBounds.✅ Suggested test additions
{"4.21 uses multi", api.VersionBounds{Lower: "4.21.0-0", Upper: "4.22.0-0"}, api.ReleaseArchitectureMULTI}, + {"5.0 uses multi", api.VersionBounds{Lower: "5.0.0-0", Upper: "5.1.0-0"}, api.ReleaseArchitectureMULTI}, + {"3.13 uses amd64", api.VersionBounds{Lower: "3.13.0-0", Upper: "3.14.0-0"}, api.ReleaseArchitectureAMD64}, {"unparseable lower uses amd64", api.VersionBounds{Lower: "bad", Upper: "4.8.0-0"}, api.ReleaseArchitectureAMD64},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/clusterimageset-updater/main_test.go` around lines 26 - 33, Add a test row to the existing table in cmd/clusterimageset-updater/main_test.go covering a non-4.x major boundary (e.g., a "5.x" range) to assert architectureForBounds handles non-4 majors consistently; update the test table (the same slice containing cases like "4.7 uses amd64") with a case such as a Lower "5.0.0-0" and Upper "5.1.0-0" expecting the default (e.g., api.ReleaseArchitectureAMD64) so architectureForBounds is exercised for non-4.x input and future regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/clusterimageset-updater/main.go`:
- Around line 331-344: architectureForBounds currently only checks the parsed
minor version from bounds.Lower and ignores the major version, causing wrong
classification for non-4.x releases; update architectureForBounds to parse both
major and minor (from parts[0] and parts[1]) as integers, handle parse errors by
returning api.ReleaseArchitectureAMD64, then return api.ReleaseArchitectureMULTI
when (major > 4) OR (major == 4 AND minor >= 12), otherwise return
api.ReleaseArchitectureAMD64; keep parsing fallback behavior and reference
bounds.Lower and the architectureForBounds function.
---
Nitpick comments:
In `@cmd/clusterimageset-updater/main_test.go`:
- Around line 26-33: Add a test row to the existing table in
cmd/clusterimageset-updater/main_test.go covering a non-4.x major boundary
(e.g., a "5.x" range) to assert architectureForBounds handles non-4 majors
consistently; update the test table (the same slice containing cases like "4.7
uses amd64") with a case such as a Lower "5.0.0-0" and Upper "5.1.0-0" expecting
the default (e.g., api.ReleaseArchitectureAMD64) so architectureForBounds is
exercised for non-4.x input and future regressions are caught.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
cmd/clusterimageset-updater/main.gocmd/clusterimageset-updater/main_test.go
deepsm007
force-pushed
the
clusterimageset-updater-multi-payload
branch
from
1241608 to
da218e6
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, hector-vido 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 |
|
/test e2e |
|
/retest |
|
@deepsm007: The following tests 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. |
3 participants
/cc @openshift/test-platform
Summary by CodeRabbit
New Features
Tests