OPRUN-4599: override catalog image tag when Helm values pin v5.0#203
OPRUN-4599: override catalog image tag when Helm values pin v5.0#203tmshort wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tmshort: This pull request references OPRUN-4599 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughCompute a catalog image tag from the operator image version, expose it on the controller Builder, conditionally inject it into rendered Helm values, and add helper + tests to format and read catalog tags as ChangesCatalog Image Tag wiring
Sequence DiagramsequenceDiagram
participant Main as cmd/cluster-olm-operator/main.go
participant VersionUtils as internal/versionutils
participant Builder as pkg/controller/builder.go
participant HelmValues as pkg/helmvalues
participant Helm as pkg/controller/helm.go
Main->>VersionUtils: GetCurrentOCPMinorVersion(operatorImageVersion)
VersionUtils-->>Main: semver.Version
Main->>VersionUtils: GetCatalogImageTag(version)
VersionUtils-->>Main: "v<major>.<minor>"
Main->>Builder: Instantiate Builder with ClusterCatalogImageTag
Builder->>Helm: renderHelmTemplate()
Helm->>HelmValues: GetStringValue("options.openshift.catalogs.version")
alt ClusterCatalogImageTag non-empty and current == "v5.0"
Helm->>HelmValues: set options.openshift.catalogs.version = ClusterCatalogImageTag
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort 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
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test openshift-e2e-aws-customnoupgrade |
tmshort
changed the title
|
/lgtm |
|
I tested this via cluster-bot: and although the OLM containers had |
|
/test openshift-e2e-aws-customnoupgrade |
|
/test openshift-e2e-aws-customnoupgrade |
|
/override ci/prow/openshift-e2e-aws-customnoupgrade |
|
@tmshort: Overrode contexts on behalf of tmshort: ci/prow/openshift-e2e-aws-customnoupgrade 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 kubernetes-sigs/prow repository. |
OCP 4.23 and 5.0 are co-released from the same branch, causing openshift.yaml to pin options.openshift.catalogs.version to "v5.0". Override the tag from RELEASE_VERSION when it equals "v5.0" so 4.23 clusters use v4.23 catalog images; all other versions are unaffected. Assisted-by: claude Signed-off-by: Todd Short <tshort@redhat.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/helm_test.go`:
- Around line 141-143: The test currently only asserts the returned value from
hv.GetStringValue (assigned to got) against tt.expectedTag and ignores the
presence flag; update the test to also check the found boolean from
hv.GetStringValue: call hv.GetStringValue(versionKey) capturing (got, found),
and for cases where tt.expectedTag is empty assert that found is false,
otherwise assert found is true and then assert got equals tt.expectedTag;
reference hv.GetStringValue, the got/found return values, and tt.expectedTag in
your changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a95c85eb-b4a9-4aad-9f37-4068d35c233c
📒 Files selected for processing (8)
cmd/cluster-olm-operator/main.gointernal/versionutils/version_utils.gointernal/versionutils/version_utils_test.gopkg/controller/builder.gopkg/controller/helm.gopkg/controller/helm_test.gopkg/helmvalues/helmvalues.gopkg/helmvalues/helmvalues_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/versionutils/version_utils.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controller/builder.go
- pkg/controller/helm.go
- pkg/helmvalues/helmvalues.go
- internal/versionutils/version_utils_test.go
- cmd/cluster-olm-operator/main.go
| got, _ := hv.GetStringValue(versionKey) | ||
| require.Equal(t, tt.expectedTag, got) | ||
| }) |
There was a problem hiding this comment.
Assert key presence, not just value, for “absent key” cases.
Line 141 ignores the found flag, but your table comment says empty expectedTag means the key is absent. A key set to "" would incorrectly pass today.
Suggested fix
- got, _ := hv.GetStringValue(versionKey)
- require.Equal(t, tt.expectedTag, got)
+ got, found := hv.GetStringValue(versionKey)
+ if tt.expectedTag == "" {
+ require.False(t, found, "expected %s to be absent", versionKey)
+ require.Empty(t, got)
+ return
+ }
+ require.True(t, found, "expected %s to be present", versionKey)
+ require.Equal(t, tt.expectedTag, got)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| got, _ := hv.GetStringValue(versionKey) | |
| require.Equal(t, tt.expectedTag, got) | |
| }) | |
| got, found := hv.GetStringValue(versionKey) | |
| if tt.expectedTag == "" { | |
| require.False(t, found, "expected %s to be absent", versionKey) | |
| require.Empty(t, got) | |
| return | |
| } | |
| require.True(t, found, "expected %s to be present", versionKey) | |
| require.Equal(t, tt.expectedTag, got) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/helm_test.go` around lines 141 - 143, The test currently only
asserts the returned value from hv.GetStringValue (assigned to got) against
tt.expectedTag and ignores the presence flag; update the test to also check the
found boolean from hv.GetStringValue: call hv.GetStringValue(versionKey)
capturing (got, found), and for cases where tt.expectedTag is empty assert that
found is false, otherwise assert found is true and then assert got equals
tt.expectedTag; reference hv.GetStringValue, the got/found return values, and
tt.expectedTag in your changes.
|
/test openshift-e2e-aws-devpreview |
|
@tmshort: 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. |
5 participants
OCP 4.23 and 5.0 are co-released from the same branch, so the catalogd
openshift.yaml Helm values file statically pins options.openshift.catalogs.version
to "v5.0". A cluster actually running 4.23 must reference v4.23 catalog index
images instead.
Compute the catalog image tag from RELEASE_VERSION at startup and, only when
options.openshift.catalogs.version equals "v5.0", override it with the tag
derived from the running cluster's OCP major.minor version. The conditional
guard keeps CI tests against other OCP versions unaffected (their published
catalog images exist under the tag shipped in the Helm values file).
A TODO marks the override block for removal once 4.23/5.0 are no longer
co-released and the values file no longer pins "v5.0".
Summary by CodeRabbit
New Features
Tests