MGMT-24398, MGMT-24399: Migrate test versions to OCP version builder#10412
Conversation
|
@bluesort: No Jira issue with key MGTM-24398 exists in the tracker at https://redhat.atlassian.net. 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
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@bluesort: No Jira issue with key MGTM-24398 exists in the tracker at https://redhat.atlassian.net. 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
the
size/XXL
|
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 a generator that parses release-images JSON into a per-architecture test-version map, implements a fluent TestVersionBuilder with tests, migrates many tests to compute OpenShift/RHCOS versions dynamically (skipping when none available), updates test defaults wiring, and refreshes OS/release image catalogs and manifests. ChangesDynamic version selection system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
openshift-ci
Bot
added
the
approved
|
@bluesort: No Jira issue with key MGTM-24398 exists in the tracker at https://redhat.atlassian.net. 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. |
|
@bluesort: No Jira issue with key MGTM-24398 exists in the tracker at https://redhat.atlassian.net. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
subsystem/feature_support_levels_test.go (1)
255-283:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove dynamic values from Ginkgo test names.
Lines 258 and 273 embed the OpenShift version string into the test name via
fmt.Sprintf. Test names must be stable and deterministic; including runtime-derived versions violates the guideline and causes test names to change when version data is updated.As per coding guidelines, Ginkgo test names must not contain "timestamps, random UUIDs, node names, namespace names with random suffixes, IP addresses, or any dynamic values that change between test runs."
🔧 Proposed fix: use stable test names
Replace the dynamic test names with stable descriptions that convey the test intent without embedding specific version strings:
- It(fmt.Sprintf("GetSupportedFeatures x86 CPU architectrue, OCP version %s", version), func() { + It("GetSupportedFeatures x86 CPU architecture with available version", func() { params = installer.GetSupportedFeaturesParams{ OpenshiftVersion: version, CPUArchitecture: swag.String(arch),- It(fmt.Sprintf("GetSupportedFeatures with empty CPU architectrue, OCP version %s", version), func() { + It("GetSupportedFeatures with empty CPU architecture and available version", func() { response, err := utils_test.TestContext.UserBMClient.Installer.GetSupportedFeatures(ctx, &installer.GetSupportedFeaturesParams{OpenshiftVersion: version})Alternatively, if you need to distinguish the test cases, use index-based names like
"GetSupportedFeatures x86 CPU architecture - oldest version"and"GetSupportedFeatures x86 CPU architecture - latest version".🤖 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 `@subsystem/feature_support_levels_test.go` around lines 255 - 283, The Ginkgo It() test names embed a dynamic OpenShift version via fmt.Sprintf (see the two It(...) calls using fmt.Sprintf and the variables availableVersions/version/arch); replace those dynamic names with stable, deterministic strings (e.g., "GetSupportedFeatures x86 CPU architecture" and "GetSupportedFeatures with empty CPU architecture") or index-based variants, removing fmt.Sprintf and any runtime version interpolation while leaving the test bodies and variables (availableVersions, version, arch, params, response) unchanged.
🧹 Nitpick comments (4)
internal/host/hostcommands/install_cmd_test.go (1)
181-189: 💤 Low valueAdditional coverage for pre-4.15 behavior is acceptable.
The new test explicitly validates that
EnableSkipMcoRebootis disabled for OpenShift 4.14. Note that the table-driven tests at lines 111-151 already cover this version boundary scenario (line 142), so this test provides redundant but harmless additional integration coverage.🤖 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/host/hostcommands/install_cmd_test.go` around lines 181 - 189, The new spec "get_step_pre_4.15_skip_mco_reboot_disabled" is redundant with the existing table-driven cases that already validate the 4.14 boundary; remove this duplicate test or consolidate its expectations into the table-driven test to avoid repetition — locate the test function named get_step_pre_4.15_skip_mco_reboot_disabled and either delete it or merge its assertions (the calls to mockValidator.GetHostInstallationPath, mockGetReleaseImage, mockImages, installCmd.GetSteps, postvalidation, and validateInstallCommand which assert EnableSkipMcoReboot behavior for "4.14") into the existing table-driven block that exercises GetSteps/validateInstallCommand.internal/common/test_versions.go (1)
95-104: ⚖️ Poor tradeoff
Oldest()/Latest()depend ontestVersionsByArchslices being pre-sorted ascending.
versions()returns the per-arch slice as-is andTryVersiontreatsv[0]as oldest andv[len-1]as latest. This is correct only because the generator emits versions sorted ascending. If the generated file is ever hand-edited or the generator's sort changes, selection silently breaks. Consider sorting inversions()(e.g. viaversion.Collection) to make the builder self-contained rather than relying on input ordering.Also applies to: 148-163
🤖 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 95 - 104, The TryVersion/Oldest/Latest behavior assumes per-arch slices in testVersionsByArch are pre-sorted; make the builder robust by sorting in TestVersionBuilder.versions before returning the slice (e.g., convert to a version.Collection and call Sort or an equivalent sort helper) so TryVersion (and Oldest()/Latest()) can safely use v[0]/v[len-1]; update TestVersionBuilder.versions to return a sorted copy of the per-arch slice rather than relying on the input order from testVersionsByArch.hack/generate.sh (1)
132-163: 💤 Low valueOptional: write to a temp file to avoid leaving a corrupt generated file on failure.
The
> "$output_file"redirection truncates the target beforepython3runs. Withset -o errexit, any Python error (e.g., malformed JSON, an entry missing the minor component soparts[1]raisesIndexError) aborts the script after the file has already been emptied, leaving a brokentest_versions_generated.go. Generating to a temp file and moving on success keeps the output atomic.♻️ Suggested refactor
python3 -c ' ... -}""" % "\n".join(lines)) -' > "$output_file" - - gofmt -w "$output_file" +}""" % "\n".join(lines)) +' > "${output_file}.tmp" + + gofmt -w "${output_file}.tmp" + mv "${output_file}.tmp" "$output_file" }🤖 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 `@hack/generate.sh` around lines 132 - 163, The script truncates "$output_file" before running the embedded Python, so any Python failure leaves a corrupted generated file; change the generation to write to a temporary file (e.g., mktemp), run the python3 block and gofmt against that temp file, then atomically mv the temp to "$output_file" on success, and ensure cleanup on exit (trap) to remove the temp on failure; reference the existing variables/data flow used in the python3 block (data_file, output_file) so the temp replace logic wraps the python3 > "$output_file" redirection and subsequent gofmt invocation.subsystem/subsystem_suite_test.go (1)
41-55: 💤 Low valueDuplicate helper bodies for
vipAutoAllocOpenshiftVersionandsdnNetworkTypeOpenshiftVersion.Both functions are byte-identical (
LessThan("4.15").TryVersion()+v + ".0", same skip message). Consider extracting a shared helper to avoid divergence, while keeping the two named call sites if they document distinct intents.♻️ Proposed refactor
-func vipAutoAllocOpenshiftVersion() string { - v, ok := common.TestVersion().LessThan("4.15").TryVersion() - if !ok { - Skip("no available version below 4.15") - } - return v + ".0" -} - -func sdnNetworkTypeOpenshiftVersion() string { - v, ok := common.TestVersion().LessThan("4.15").TryVersion() - if !ok { - Skip("no available version below 4.15") - } - return v + ".0" -} +func versionBelow415() string { + v, ok := common.TestVersion().LessThan("4.15").TryVersion() + if !ok { + Skip("no available version below 4.15") + } + return v + ".0" +} + +func vipAutoAllocOpenshiftVersion() string { return versionBelow415() } +func sdnNetworkTypeOpenshiftVersion() string { return versionBelow415() }🤖 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 `@subsystem/subsystem_suite_test.go` around lines 41 - 55, Extract the duplicated logic in vipAutoAllocOpenshiftVersion and sdnNetworkTypeOpenshiftVersion into a single shared helper (e.g., openshiftVersionBelow415) that calls common.TestVersion().LessThan("4.15").TryVersion(), performs the same Skip("no available version below 4.15") on !ok, and returns v + ".0"; then have vipAutoAllocOpenshiftVersion and sdnNetworkTypeOpenshiftVersion simply call that helper to preserve the two distinct named call sites and intent while avoiding duplicated bodies.
🤖 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 `@subsystem/agent_based_installer_client_test.go`:
- Line 18: Replace the "// TODO: comment" placeholder with a concise Go doc
comment for the function versionFromClusterImageSet: describe its purpose,
inputs, outputs and any important behavior (e.g., how it derives a version from
a ClusterImageSet, expected return values and error conditions). Ensure the
comment starts with the function name "versionFromClusterImageSet" so it becomes
a proper Go doc string above that function; alternatively remove the TODO
entirely if you decide the function name is sufficiently self-explanatory.
In `@subsystem/cluster_test.go`:
- Around line 3730-3735: The test registers a cluster using
common.TestVersion().GreaterThanOrEqual("4.12").ReleaseVersion() which can panic
if no matching test version exists; wrap the version selection with TryVersion()
and call Skip(...) when TryVersion returns nil, then pass the found version's
ReleaseVersion() into models.ClusterCreateParams.OpenshiftVersion before calling
TestContext.UserBMClient.Installer.V2RegisterCluster; follow the existing
TryVersion() + Skip(...) pattern used elsewhere in this file to guard the
registration path.
In `@subsystem/kubeapi/kubeapi_test.go`:
- Around line 5543-5544: The test computes initialVersion :=
common.TestVersion().Oldest().Version() and updatedVersion :=
common.TestVersion().Version() and then asserts a URL change; if the dataset has
only a single version those two can be equal and the assertion will flake. Add a
guard around the URL-change assertion (or early return/Skip for the test) that
checks if initialVersion == updatedVersion and if so skips or short-circuits the
URL-change assertion; apply the same check where the test repeats the comparison
(the other occurrence around updatedVersion usage).
- Around line 138-144: Global var initialization eagerly calls
common.TestVersion().ForArch("arm64") and ForArch("multi") which can panic if
those arches are missing; move the imageSetNameArm/imageSetNameMulti and
imageSetsData construction out of package-level init into a guarded init() (or a
helper function) that calls common.TestVersion().ForArch(...) and checks for a
nil/missing result before using .Version()/.ReleaseImageURL(), only adding
entries for arches that exist; reference imageSetNameArm, imageSetNameMulti,
imageSetsData and common.TestVersion().ForArch in your change so the creation
becomes lazy/safe and avoids package-init panics.
---
Outside diff comments:
In `@subsystem/feature_support_levels_test.go`:
- Around line 255-283: The Ginkgo It() test names embed a dynamic OpenShift
version via fmt.Sprintf (see the two It(...) calls using fmt.Sprintf and the
variables availableVersions/version/arch); replace those dynamic names with
stable, deterministic strings (e.g., "GetSupportedFeatures x86 CPU architecture"
and "GetSupportedFeatures with empty CPU architecture") or index-based variants,
removing fmt.Sprintf and any runtime version interpolation while leaving the
test bodies and variables (availableVersions, version, arch, params, response)
unchanged.
---
Nitpick comments:
In `@hack/generate.sh`:
- Around line 132-163: The script truncates "$output_file" before running the
embedded Python, so any Python failure leaves a corrupted generated file; change
the generation to write to a temporary file (e.g., mktemp), run the python3
block and gofmt against that temp file, then atomically mv the temp to
"$output_file" on success, and ensure cleanup on exit (trap) to remove the temp
on failure; reference the existing variables/data flow used in the python3 block
(data_file, output_file) so the temp replace logic wraps the python3 >
"$output_file" redirection and subsequent gofmt invocation.
In `@internal/common/test_versions.go`:
- Around line 95-104: The TryVersion/Oldest/Latest behavior assumes per-arch
slices in testVersionsByArch are pre-sorted; make the builder robust by sorting
in TestVersionBuilder.versions before returning the slice (e.g., convert to a
version.Collection and call Sort or an equivalent sort helper) so TryVersion
(and Oldest()/Latest()) can safely use v[0]/v[len-1]; update
TestVersionBuilder.versions to return a sorted copy of the per-arch slice rather
than relying on the input order from testVersionsByArch.
In `@internal/host/hostcommands/install_cmd_test.go`:
- Around line 181-189: The new spec "get_step_pre_4.15_skip_mco_reboot_disabled"
is redundant with the existing table-driven cases that already validate the 4.14
boundary; remove this duplicate test or consolidate its expectations into the
table-driven test to avoid repetition — locate the test function named
get_step_pre_4.15_skip_mco_reboot_disabled and either delete it or merge its
assertions (the calls to mockValidator.GetHostInstallationPath,
mockGetReleaseImage, mockImages, installCmd.GetSteps, postvalidation, and
validateInstallCommand which assert EnableSkipMcoReboot behavior for "4.14")
into the existing table-driven block that exercises
GetSteps/validateInstallCommand.
In `@subsystem/subsystem_suite_test.go`:
- Around line 41-55: Extract the duplicated logic in
vipAutoAllocOpenshiftVersion and sdnNetworkTypeOpenshiftVersion into a single
shared helper (e.g., openshiftVersionBelow415) that calls
common.TestVersion().LessThan("4.15").TryVersion(), performs the same Skip("no
available version below 4.15") on !ok, and returns v + ".0"; then have
vipAutoAllocOpenshiftVersion and sdnNetworkTypeOpenshiftVersion simply call that
helper to preserve the two distinct named call sites and intent while avoiding
duplicated bodies.
🪄 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: 99616945-b4ae-4500-84a8-2072ab5f0785
📒 Files selected for processing (24)
data/default_os_images.jsondata/default_release_images.jsonhack/Makefilehack/generate.shinternal/bminventory/inventory_test.gointernal/common/test_configuration.gointernal/common/test_versions.gointernal/common/test_versions_generated.gointernal/common/test_versions_test.gointernal/host/hostcommands/install_cmd_test.gosubsystem/agent_based_installer_client_test.gosubsystem/ams_subscriptions_test.gosubsystem/authz_test.gosubsystem/cluster_test.gosubsystem/cluster_v2_test.gosubsystem/feature_support_levels_test.gosubsystem/host_test.gosubsystem/infra_env_test.gosubsystem/kubeapi/kubeapi_suite_test.gosubsystem/kubeapi/kubeapi_test.gosubsystem/metrics_test.gosubsystem/operators_test.gosubsystem/subsystem_suite_test.gosubsystem/utils_test/subsystem_test_context.go
💤 Files with no reviewable changes (1)
- subsystem/kubeapi/kubeapi_suite_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10412 +/- ##
==========================================
- Coverage 44.35% 43.88% -0.48%
==========================================
Files 420 420
Lines 73107 75195 +2088
==========================================
+ Hits 32426 32998 +572
- Misses 37744 39256 +1512
- Partials 2937 2941 +4
🚀 New features to boost your workflow:
|
|
@bluesort: No Jira issue with key MGTM-24398 exists in the tracker at https://redhat.atlassian.net. 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. |
|
@bluesort: No Jira issue with key MGTM-24398 exists in the tracker at https://redhat.atlassian.net. 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. |
bluesort
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bluesort: This pull request references MGMT-24398 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. |
|
@bluesort: This pull request references MGMT-24398 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. |
bluesort
changed the title
|
@bluesort: This pull request references MGMT-24398 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. This pull request references MGMT-24399 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
the
needs-rebase
bluesort
force-pushed
the
mgmt-24398
branch
2 times, most recently
from
4b041dd to
7139ac7
Compare
openshift-ci
Bot
added
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
bluesort
force-pushed
the
mgmt-24398
branch
3 times, most recently
from
ffa5600 to
dce36f2
Compare
|
Did you try running the tests with your changes in a scenario where some of the versions don't exist? If not, it would be good to test that |
|
How do we ensure that future tests follow this approach instead of using hardcoded constants as we did before? Maybe some CI validation could help? though that might be overkill? need to think about that |
I pruned versions down to the set required for release-ocm-2.17 to simulate a release branch to confirm test version boundary tests behave as expected. The PR description has a bit more context |
I'm adding documentation as a separate subtask and plan to add a dedicated subsystem test writing skill (similar to the existing unit testing one) to enforce the use of this pattern. I tend to think CI enforcement should be a last resort, so I'd like to see how much of an issue it is first |
bluesort
force-pushed
the
mgmt-24398
branch
2 times, most recently
from
7e017ee to
75e2885
Compare
|
/retest |
|
/lgtm |
|
/test e2e-agent-compact-ipv4 |
|
/override ci/prow/e2e-agent-compact-ipv4 |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/e2e-agent-compact-ipv4 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. |
This pairs with the prior commit (MGMT-24397) which introduced the builder itself. Assisted-by: Claude Code <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bluesort, linoyaslan 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 |
|
@bluesort: 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. |
3 participants
Migrates subsystem tests and default unit test configuration to use the
TestVersionBuilderintroduced in MGMT-24397.This PR covers both MGMT-24398 and MGMT-24399 since they modify the same files.
To confirm resilience to release branch version pruning, older versions were temporarily removed from this branch. All tests targeting specific version ranges correctly skipped when their constraints were no longer satisfied, and all remaining tests passed. The version data changes were then reverted for merge.
Key changes
test_versions_generated.gowas previously a flat list of version strings per arch. It now storestestOsImage(version + URL) andtestReleaseImage(version + URL) maps sourced fromdefault_os_images.jsonanddefault_release_images.json. Methods likeReleaseImageURL(),RhcosImageURL(),RhcosVersion(), andReleaseVersion()return real values from the generated data rather than synthesizing URLs from a pattern.TestVersion(). All subsystem tests now use the builder's constraint API (LessThan,GreaterThanOrEqual,Exact, etc.) withTryVersion()/Skip()to gracefully handle version-boundary tests. Tests that require a specific OCP version range skip rather than fail when the version set doesn't include a match.SubsystemTestContextand kubeapi tests now derive versions fromTestVersion()instead of hardcoded"4.16"/"4.12"strings.agent_based_installer_client_test.goextracts its expected version from theclusterImageSet.yamlfixture to stay in sync when the fixture is updated.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
Bug Fixes
Chores