MGMT-24397: Add OCP test version builder & PoC#10321
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@bluesort: This pull request references MGMT-24397 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 sub-task 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. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bluesort 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds generation of architecture-scoped OpenShift test versions from release-images data, a fluent Go API to select/filter versions, wires a DefaultTestVersion into test configuration, updates tests to use dynamic versions, and registers a Make target to run the generator. ChangesTest version generation, selection API, wiring, and test updates
sequenceDiagram
participant Make as Make
participant GenScript as hack/generate.sh
participant Data as data/default_release_images.json
participant GenFile as internal/common/test_versions_generated.go
participant GoPkg as internal/common (test_versions.go)
participant Tests as test files
Make->>GenScript: run generate_test_versions
GenScript->>Data: read release images data
GenScript->>GenFile: write testVersionsByArch (generated Go)
GenScript->>GenFile: run gofmt
GoPkg->>GenFile: read testVersionsByArch at runtime
Tests->>GoPkg: call TestVersion().Latest()/Exact()/ReleaseImageURL()
GoPkg-->>Tests: return computed versions and image URLs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/common/test_versions.go`:
- Around line 51-55: The Find() method on TestVersionBuilder exposes a boolean
miss path but currently panics on the unknown-arch branch; update
TestVersionBuilder.Find (and the similar branch referenced around lines 83-87)
to avoid panics by returning ("", false) for any unknown/unsupported
architecture instead of calling panic, ensuring all code paths conform to the
advertised safe return contract and callers can probe availability without
crashing.
- Around line 70-81: ReleaseVersion() and ReleaseImageURL() always append ".0"
and "-<arch>" blindly, causing invalid tags for inputs like "4.11.0-multi";
update TestVersionBuilder.ReleaseVersion and ReleaseImageURL to 1) compute base
:= b.MustFind() and only append ".0" if the base has only two numeric components
(e.g., count of '.' == 1), 2) map ARM64CPUArchitecture -> AARCH64CPUArchitecture
for the arch variable as before, but only append "-<arch>" to the tag in
ReleaseImageURL if the base does not already contain a hyphen-suffix (i.e.,
strings.Contains(base, "-") == false) and the arch is non-empty; use the
existing symbols ReleaseVersion(), ReleaseImageURL(), MustFind(), arch,
ARM64CPUArchitecture, AARCH64CPUArchitecture to implement these checks.
🪄 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: 9ade2e0b-ecea-42ad-b18a-4cbd13315e0f
📒 Files selected for processing (6)
hack/Makefilehack/generate.shinternal/common/test_configuration.gointernal/common/test_versions.gointernal/common/test_versions_generated.gointernal/releasesources/release_sources_test.go
bluesort
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/common/test_versions.go (1)
69-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease tag construction is still too rigid for already-qualified versions.
Line 70 always appends
.0, and Line 79 always appends-<arch>. If the selected version already includes patch/suffix (for examplex.y.zorx.y.z-multi), the tag becomes malformed.Proposed fix
import ( "fmt" + "strings" "github.com/hashicorp/go-version" ) @@ func (b *TestVersionBuilder) ReleaseVersion() string { - return b.Version() + ".0" + v := b.Version() + base := strings.TrimSuffix(v, "-multi") + if strings.Count(base, ".") == 1 { + base += ".0" + } + if strings.HasSuffix(v, "-multi") { + return base + "-multi" + } + return base } @@ func (b *TestVersionBuilder) ReleaseImageURL() string { + if b.arch == MultiCPUArchitecture { + return fmt.Sprintf("quay.io/openshift-release-dev/ocp-release:%s", b.ReleaseVersion()) + } suffix := b.arch if suffix == ARM64CPUArchitecture { suffix = AARCH64CPUArchitecture } return fmt.Sprintf("quay.io/openshift-release-dev/ocp-release:%s-%s", b.ReleaseVersion(), suffix) }🤖 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 `@internal/common/test_versions.go` around lines 69 - 80, The ReleaseVersion and ReleaseImageURL helpers currently always append ".0" and "-<arch>", which corrupts already-qualified versions like "x.y.z" or "x.y.z-multi"; update ReleaseVersion to only append ".0" when b.Version() is a bare "major.minor" (e.g., match something like `^\d+\.\d+$`) and otherwise return b.Version() unchanged, and update ReleaseImageURL to build the tag from ReleaseVersion but only append the architecture suffix when the tag does not already contain a "-" suffix (i.e., if the tag already contains "-" or already ends with the mapped arch string, do not add another "-<arch>"); keep the ARM64->AARCH64 mapping logic using ARM64CPUArchitecture and AARCH64CPUArchitecture and use fmt.Sprintf as before.
🤖 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.
Duplicate comments:
In `@internal/common/test_versions.go`:
- Around line 69-80: The ReleaseVersion and ReleaseImageURL helpers currently
always append ".0" and "-<arch>", which corrupts already-qualified versions like
"x.y.z" or "x.y.z-multi"; update ReleaseVersion to only append ".0" when
b.Version() is a bare "major.minor" (e.g., match something like `^\d+\.\d+$`)
and otherwise return b.Version() unchanged, and update ReleaseImageURL to build
the tag from ReleaseVersion but only append the architecture suffix when the tag
does not already contain a "-" suffix (i.e., if the tag already contains "-" or
already ends with the mapped arch string, do not add another "-<arch>"); keep
the ARM64->AARCH64 mapping logic using ARM64CPUArchitecture and
AARCH64CPUArchitecture and use fmt.Sprintf as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1626b3a5-de76-42f3-a977-cace49fa3f45
📒 Files selected for processing (3)
hack/generate.shinternal/common/test_versions.gointernal/common/test_versions_generated.go
✅ Files skipped from review due to trivial changes (1)
- internal/common/test_versions_generated.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/generate.sh
bluesort
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
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 `@hack/Makefile`:
- Line 67: The Makefile currently defines targets named generate and
generate-test-versions that should be declared phony to avoid accidental
skipping when files with those names exist; update the Makefile by adding a
.PHONY declaration listing generate and generate-test-versions (e.g., add a line
like ".PHONY: generate generate-test-versions" near other .PHONY entries or the
top of the file) so that the generate and generate-test-versions targets are
always executed regardless of same-named files in the workspace.
🪄 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: 7795b9bb-6c76-43f4-9cf5-3d644cf26f16
📒 Files selected for processing (7)
hack/Makefilehack/generate.shinternal/common/test_configuration.gointernal/common/test_versions.gointernal/common/test_versions_generated.gointernal/releasesources/release_sources_test.gosubsystem/cluster_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/common/test_versions_generated.go
🚧 Files skipped from review as they are similar to previous changes (4)
- hack/generate.sh
- internal/common/test_versions.go
- internal/releasesources/release_sources_test.go
- internal/common/test_configuration.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10321 +/- ##
==========================================
+ Coverage 44.33% 44.37% +0.03%
==========================================
Files 417 418 +1
Lines 72837 72829 -8
==========================================
+ Hits 32294 32316 +22
+ Misses 37609 37592 -17
+ Partials 2934 2921 -13
🚀 New features to boost your workflow:
|
|
/override ci/prow/edge-e2e-ai-operator-ztp |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/edge-e2e-ai-operator-ztp 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. |
|
/test mce-images |
openshift-ci
Bot
added
size/XL
|
/override ci/prow/edge-e2e-ai-operator-ztp
|
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/edge-e2e-ai-operator-ztp 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. |
|
typo |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/verify-generated-code 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. |
Add TestVersionBuilder for resolving test versions from data files instead of hardcoding them. These helpers let tests reference versions dynamically so they adapt when data files change via make generate.
|
/test edge-e2e-metal-assisted-4-22 |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/edge-e2e-ai-operator-ztp, ci/prow/verify-generated-code 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. |
|
/retest |
|
@bluesort: 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. |
2 participants
Problem
During ACM feature freeze, older OCP versions are pruned from data files. Tests that hardcode version strings then break and require manual edits or removal to reach green CI on the version pruning PR.
Solution
Add TestVersionBuilder for resolving test versions from data files instead of hardcoding them. This API lets tests reference versions dynamically, applying any needed constraints, so they adapt when data files change via
make generate.Tests that are specific to exact versions or ranges will specify constrains and will be skipped when no matching version is present. 2 of the tests in
subsystem/cluster_test.gohave been updated as a PoC for this pattern.Unit tests unnecessarily pinned to specific versions will be updated to use the latest available version or apply their own constraints and skip when no matching version is present. The shared unit test config as well as the tests in
internal/releasesources/release_sources_test.gohave been updated as a PoC for this pattern.The rest of the codebase will be updated in followup branches to keep changes focused and testable.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
New Features
Tests
Bug Fixes