OPRUN-4627: override catalog tag to 4.x when RELEASE_VERSION is 4.x and catalogd.yaml pins v5.0#210
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tmshort: This pull request references OPRUN-4627 which is a valid 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: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughCatalog-tag computation was moved out of Builder wiring; Builder no longer stores ClusterCatalogImageTag. GetCatalogImageTag now special-cases operator 5.1 to return v5.0 and its prior table-driven test was removed. Helm rendering uses RELEASE_VERSION-driven inline logic and tests were updated to drive behavior via RELEASE_VERSION. ChangesCatalog tag and operator wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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
| // TODO: Remove after 4.23/5.0 branch cut; on the main branch replace "ocp-release" in | ||
| // openshift/helm/catalogd.yaml (github.com/openshift/operator-framework-operator-controller) with "v5.0". | ||
| // OCP 5.1 uses v5.0 catalog images until the 5.1 catalogs are available. | ||
| if version.Major == 5 && version.Minor == 1 { | ||
| return "v5.0" | ||
| } | ||
| return fmt.Sprintf("v%d.%d", version.Major, version.Minor) |
There was a problem hiding this comment.
Something that was brought up today by @grokspawn. Instead of using ocp-release as a sentinel for "auto-version", it might be better to have specific code that handles just the "map 5.0 to 4.23" case:
name, tag := extractNameTag(imageRef)
if tag == "v5.0" and currentClusterVersion == "4.23" {
tag = "v4.23"
}
imageRef = makeImageRef(name, tag)If we did that, 5.1 would use 5.0 automatically until we change it, just like we've always done.
There was a problem hiding this comment.
I hear you. It's still code that needs to be deleted; one we branch, and are past 4.23 and no longer need this code.
The ocp-release sentinel could be reused; which is why I implemented it as generic.
Unfortunately, it doesn't seem to be a solution that isn't temporary and becomes tech-debt almost immediately and then needs to be deleted. I'm really disliking all solutions right now (says the person who doesn't want to deal with temporary, special-case code).
There was a problem hiding this comment.
Yeah, I think it's tech debt no matter what. But it seems like even more tech debt to have code that handles versions outside of 5.0 and 4.23. Seems like the approach Jordan and I were mulling over minimizes the tech debt and keeps our normal processes otherwise undisturbed?
There was a problem hiding this comment.
I basically think I did as suggested.
|
/retest |
tmshort
force-pushed
the
handle-5.1-special-case
branch
from
abda1ca to
03c183e
Compare
tmshort
changed the title
tmshort
force-pushed
the
handle-5.1-special-case
branch
from
03c183e to
fd19ccc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/helm.go (1)
84-99: ⚡ Quick winExtract the catalog-tag rewrite into a shared helper.
This block is now duplicated in
TestCatalogImageTagOverride, so the test can drift with the implementation and still stay green. Pulling it into a small helper would trim the nesting here and let the test exercise the same code path.♻️ Proposed refactor
+func rewriteCatalogTagForRelease(values *helmvalues.HelmValues, releaseVersion string) error { + if !strings.HasPrefix(releaseVersion, "4.") { + return nil + } + + currentCatalogVersion, found := values.GetStringValue("options.openshift.catalogs.version") + if !found || currentCatalogVersion != "v5.0" { + return nil + } + + v, err := versionutils.GetCurrentOCPMinorVersion(releaseVersion) + if err != nil { + return fmt.Errorf("error parsing release version for catalog tag: %w", err) + } + + if err := values.SetStringValue("options.openshift.catalogs.version", fmt.Sprintf("v%d.%d", v.Major, v.Minor)); err != nil { + return fmt.Errorf("error setting catalog image tag: %w", err) + } + return nil +} + releaseVersion := os.Getenv("RELEASE_VERSION") - if strings.HasPrefix(releaseVersion, "4.") { - currentCatalogVersion, found := values.GetStringValue("options.openshift.catalogs.version") - if found && currentCatalogVersion == "v5.0" { - v, err := versionutils.GetCurrentOCPMinorVersion(releaseVersion) - if err != nil { - return fmt.Errorf("error parsing release version for catalog tag: %w", err) - } - if err := values.SetStringValue("options.openshift.catalogs.version", fmt.Sprintf("v%d.%d", v.Major, v.Minor)); err != nil { - return fmt.Errorf("error setting catalog image tag: %w", err) - } - } + if err := rewriteCatalogTagForRelease(values, releaseVersion); err != nil { + return err }🤖 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.go` around lines 84 - 99, Extract the duplicated catalog-tag rewrite logic into a small shared helper (e.g. func rewriteCatalogImageTag(values Values, releaseVersion string) error) that encapsulates the checks for strings.HasPrefix(releaseVersion, "4."), reading "options.openshift.catalogs.version" via values.GetStringValue, parsing the OCP minor via versionutils.GetCurrentOCPMinorVersion, and calling values.SetStringValue with the computed v{Major}.{Minor}; replace the inline block in helm.go with a call to that helper and update TestCatalogImageTagOverride to call/assert the same helper so both test and implementation exercise identical code paths and return/propagate any errors unchanged.
🤖 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.
Nitpick comments:
In `@pkg/controller/helm.go`:
- Around line 84-99: Extract the duplicated catalog-tag rewrite logic into a
small shared helper (e.g. func rewriteCatalogImageTag(values Values,
releaseVersion string) error) that encapsulates the checks for
strings.HasPrefix(releaseVersion, "4."), reading
"options.openshift.catalogs.version" via values.GetStringValue, parsing the OCP
minor via versionutils.GetCurrentOCPMinorVersion, and calling
values.SetStringValue with the computed v{Major}.{Minor}; replace the inline
block in helm.go with a call to that helper and update
TestCatalogImageTagOverride to call/assert the same helper so both test and
implementation exercise identical code paths and return/propagate any errors
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b783ce46-d6f3-408a-a1ba-c105744c391e
📒 Files selected for processing (6)
cmd/cluster-olm-operator/main.gointernal/versionutils/version_utils.gointernal/versionutils/version_utils_test.gopkg/controller/builder.gopkg/controller/helm.gopkg/controller/helm_test.go
💤 Files with no reviewable changes (2)
- internal/versionutils/version_utils_test.go
- internal/versionutils/version_utils.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/helm.go (1)
84-99: ⚡ Quick winExtract the catalog-override block into a helper to clear the
nestiflint error.The logic is correct and matches the PR intent. However, golangci-lint flags this block as exceeding the
nestifcomplexity threshold (5), which can fail CI. Re-extracting it into a small helper (similar to the removedapplyCatalogImageTagOverride) keepsrenderHelmTemplatereadable and resolves the lint error.♻️ Proposed extraction
- // OCP 4.23 and 5.0 are co-released. If catalogd.yaml pins "v5.0" but RELEASE_VERSION is - // a 4.x stream, substitute the 4.x catalog tag so images match the release stream. - // For 5.x releases (including 5.1) the value in catalogd.yaml is left untouched. - releaseVersion := os.Getenv("RELEASE_VERSION") - if strings.HasPrefix(releaseVersion, "4.") { - currentCatalogVersion, found := values.GetStringValue("options.openshift.catalogs.version") - if found && currentCatalogVersion == "v5.0" { - v, err := versionutils.GetCurrentOCPMinorVersion(releaseVersion) - if err != nil { - return fmt.Errorf("error parsing release version for catalog tag: %w", err) - } - if err := values.SetStringValue("options.openshift.catalogs.version", fmt.Sprintf("v%d.%d", v.Major, v.Minor)); err != nil { - return fmt.Errorf("error setting catalog image tag: %w", err) - } - } - } + if err := applyCatalogImageTagOverride(values); err != nil { + return err + }Add the helper (e.g. near the other package functions):
// applyCatalogImageTagOverride substitutes the 4.x catalog tag when catalogd.yaml // pins "v5.0" but RELEASE_VERSION is a 4.x stream (4.23 and 5.0 are co-released). // For 5.x releases the value is left untouched. func applyCatalogImageTagOverride(values *helmvalues.HelmValues) error { releaseVersion := os.Getenv("RELEASE_VERSION") if !strings.HasPrefix(releaseVersion, "4.") { return nil } currentCatalogVersion, found := values.GetStringValue("options.openshift.catalogs.version") if !found || currentCatalogVersion != "v5.0" { return nil } v, err := versionutils.GetCurrentOCPMinorVersion(releaseVersion) if err != nil { return fmt.Errorf("error parsing release version for catalog tag: %w", err) } if err := values.SetStringValue("options.openshift.catalogs.version", fmt.Sprintf("v%d.%d", v.Major, v.Minor)); err != nil { return fmt.Errorf("error setting catalog image tag: %w", err) } return nil }🤖 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.go` around lines 84 - 99, The nested-if catalog override block should be moved into a new helper function (e.g. applyCatalogImageTagOverride(values *helmvalues.HelmValues) error) to satisfy the nestif lint rule; implement the helper to read RELEASE_VERSION, early-return nil if it doesn't start with "4.", check options.openshift.catalogs.version and early-return if missing or not "v5.0", call versionutils.GetCurrentOCPMinorVersion and handle/return errors, and set the catalog version via values.SetStringValue; then replace the original block in renderHelmTemplate (or the function containing it) with a single call to applyCatalogImageTagOverride(values) and propagate any error.
🤖 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.
Nitpick comments:
In `@pkg/controller/helm.go`:
- Around line 84-99: The nested-if catalog override block should be moved into a
new helper function (e.g. applyCatalogImageTagOverride(values
*helmvalues.HelmValues) error) to satisfy the nestif lint rule; implement the
helper to read RELEASE_VERSION, early-return nil if it doesn't start with "4.",
check options.openshift.catalogs.version and early-return if missing or not
"v5.0", call versionutils.GetCurrentOCPMinorVersion and handle/return errors,
and set the catalog version via values.SetStringValue; then replace the original
block in renderHelmTemplate (or the function containing it) with a single call
to applyCatalogImageTagOverride(values) and propagate any error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3bb85073-1ff1-4006-af4a-65c081aa8b15
📒 Files selected for processing (6)
cmd/cluster-olm-operator/main.gointernal/versionutils/version_utils.gointernal/versionutils/version_utils_test.gopkg/controller/builder.gopkg/controller/helm.gopkg/controller/helm_test.go
💤 Files with no reviewable changes (2)
- internal/versionutils/version_utils_test.go
- internal/versionutils/version_utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/controller/builder.go
- pkg/controller/helm_test.go
- cmd/cluster-olm-operator/main.go
|
/retest |
…yaml pins v5.0 Partially reverts commit 7a85fa8: removes the ocp-release sentinel mechanism (GetCatalogImageTag, ClusterCatalogImageTag, applyCatalogImageTagOverride, and the associated Builder field and main.go changes). Replaces it with a targeted check in renderHelmTemplate: when openshift/helm/catalogd.yaml (operator-framework-operator-controller) pins options.openshift.catalogs.version to "v5.0" and RELEASE_VERSION is a 4.x stream (the 4.23/5.0 co-release), substitute the 4.x catalog tag so images match the release stream. For 5.x releases, including 5.1 running against a "v5.0" or "v5.1" catalog, the value in catalogd.yaml is left untouched. Assisted-by: Claude code Signed-off-by: Todd Short <tshort@redhat.com>
tmshort
force-pushed
the
handle-5.1-special-case
branch
from
fd19ccc to
f20817c
Compare
|
/test images |
|
/retest |
1 similar comment
|
/retest |
|
/override ci/prow/openshift-e2e-aws-customnoupgrade This is unable to install, through no fault of OLM |
|
@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. |
|
/verified by @ankitathomas
|
openshift-ci-robot
added
the
verified
|
@ankitathomas: This PR has been marked as verified by 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. |
|
@tmshort: all tests passed! 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. |
|
/lgtm |
5 participants
Partially reverts #203: removes the ocp-release sentinel mechanism (GetCatalogImageTag, ClusterCatalogImageTag, applyCatalogImageTagOverride, and the associated Builder field and main.go changes).
Replaces it with a targeted check in renderHelmTemplate: when openshift/helm/catalogd.yaml (operator-framework-operator-controller) pins options.openshift.catalogs.version to "v5.0" and RELEASE_VERSION is a 4.x stream (the 4.23/5.0 co-release), substitute the 4.x catalog tag so images match the release stream. For 5.x releases, including 5.1 running against a "v5.0" or "v5.1" catalog, the value in catalogd.yaml is left untouched.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests