USHIFT-6718: major version changes for test framework#6372
Open
pacevedom wants to merge 1 commit intoopenshift:mainfrom
Open
USHIFT-6718: major version changes for test framework#6372pacevedom wants to merge 1 commit intoopenshift:mainfrom
pacevedom wants to merge 1 commit intoopenshift:mainfrom
Conversation
WalkthroughDerive OpenShift major and minor from version sources and propagate them across scripts, templates, and tests; replace hardcoded Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
Author
|
/test ? |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom 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
Contributor
Author
|
/test e2e-aws-tests |
Contributor
Author
|
/test e2e-aws-tests |
Contributor
Author
|
/test e2e-aws-testes |
Contributor
Author
|
/test e2e-aws-tests |
pacevedom
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@pacevedom: This pull request references USHIFT-6718 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 "4.22.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. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/devenv-builder/configure-vm.sh`:
- Around line 279-282: The OCP_MAJOR/OCP_MINOR extraction is using cut on the
file path ${MAKE_VERSION} instead of the value read from the file; change it to
follow the pattern used at line 197 by first extracting MAKE_VERSION from the
file using grep + cut -d'=' -f2 (trimming whitespace), then parse that value
with cut -d'.' -f1 and -f2 to set OCP_MAJOR and OCP_MINOR, set
OCPVERSION="${OCP_MAJOR}.${OCP_MINOR}", and keep OCC_SRC construction the same;
update the variable names (MAKE_VERSION, OCP_MAJOR, OCP_MINOR, OCPVERSION,
OCC_SRC) where they are assigned so you parse the actual version string rather
than the file path.
In `@scripts/get-latest-rhocp-repo.sh`:
- Around line 47-48: The calculation of stop from current_minor can produce a
negative value which later generates invalid repo names; update the logic around
variables current_minor and stop so stop is clamped to a minimum of 0 before the
repo scan loop (the variable used in the for/seq loop around the lines that
iterate over minor versions). Locate the assignment to stop and replace it with
a safe clamp (e.g., set stop to 0 when current_minor - 3 is negative) so
subsequent checks that build repo URLs using current_minor and stop cannot
produce negative minors.
In `@scripts/release-notes/gen_gh_releases_from_mirror.py`:
- Around line 172-182: The code hardcodes url_base_aarch64 and url_base_x86
using the current branch major and then scans all versions with those bases,
which breaks when versions_to_scan contains a different major; update the calls
to find_new_releases so the mirror URL base is computed per version: for each
version in versions_to_scan determine its major (e.g., parse version -> major)
and call get_mirror_url_base(major_for_version, 'aarch64') and
get_mirror_url_base(major_for_version, 'x86_64') and then pass those per-version
url bases into find_new_releases (when invoking from the blocks that use args.ec
and args.rc) so each scanned version queries the correct openshift-v{major}
tree.
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 284-287: get_gitops_version() currently assumes compatibility with
major 4 and only decrements the minor, which breaks when
generate_common_versions() computes cross-major Y-1/Y-2 (e.g., --major 5 --minor
0). Update get_gitops_version() to be major-aware: accept or derive both major
and minor, and when probing fallback versions iterate using
get_previous_version(major, minor) (or the same logic used in
generate_common_versions) to step to previous minor/major pairs instead of just
decrementing minor; ensure the same change is applied for the other lookup block
referenced around lines 335-337 so all GITOPS_VERSION lookups use the
cross-major-aware fallback sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f0660fe4-2926-4a4e-aede-d055b2097a60
📒 Files selected for processing (19)
scripts/devenv-builder/configure-vm.shscripts/get-latest-rhocp-repo.shscripts/release-notes/gen_gh_releases_from_mirror.pyscripts/release-notes/gen_gh_releases_from_rhocp.pytest/assets/common_versions.sh.templatetest/bin/build_rpms.shtest/bin/common.shtest/bin/common_versions.shtest/bin/pyutils/build_bootc_images.pytest/bin/pyutils/generate_common_versions.pytest/bin/scenario.shtest/kickstart-templates/includes/post-cos9rpm.cfgtest/package-sources/rhocp-y.tomltest/package-sources/rhocp-y1.tomltest/package-sources/rhocp-y2.tomltest/scenarios/presubmits/el98-src@rpm-install.sh.disabledtest/scenarios/releases/el98@rpm-standard1.sh.disabledtest/scenarios/releases/el98@rpm-standard2.sh.disabledtest/scenarios/releases/el98@rpm-upgrade.sh.disabled
pacevedom
force-pushed
the
test-framework-5.0
branch
from
3c69948 to
c6a53ad
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/bin/pyutils/generate_common_versions.py (1)
275-285:⚠️ Potential issue | 🟡 MinorAdd try/except for
get_previous_versionin gitops lookup.If the loop iterates close to a major boundary (e.g., starting at 4.1), line 284 will raise
KeyErrorwhen reaching 4.0's previous version.Proposed fix
for _ in range(4): for gitops_version_from_api_docs in data.get("data", [{}])[0].get("versions", []): gitops_version_ocp_compatibility = gitops_version_from_api_docs.get("openshift_compatibility") or "" gitops_version_number = gitops_version_from_api_docs.get("name") if f"{current_major}.{current_minor}" in gitops_version_ocp_compatibility: logging.info(f"Latest GitOps version: {gitops_version_number} which is compatible with OCP {gitops_version_ocp_compatibility}") return gitops_version_number - current_major, current_minor = get_previous_version(current_major, current_minor) + try: + current_major, current_minor = get_previous_version(current_major, current_minor) + except KeyError: + break return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` around lines 275 - 285, The loop that walks previous versions calls get_previous_version(current_major, current_minor) without handling KeyError; wrap that call in a try/except KeyError around the invocation in the loop in generate_common_versions.py, log or handle the error (e.g., logging.warning and break or return an empty string) and stop iterating when a KeyError occurs so the function returns "" instead of raising; ensure you keep the existing behavior of returning gitops_version_number when found and returning "" on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 275-285: The loop that walks previous versions calls
get_previous_version(current_major, current_minor) without handling KeyError;
wrap that call in a try/except KeyError around the invocation in the loop in
generate_common_versions.py, log or handle the error (e.g., logging.warning and
break or return an empty string) and stop iterating when a KeyError occurs so
the function returns "" instead of raising; ensure you keep the existing
behavior of returning gitops_version_number when found and returning "" on
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: acadad9a-5a8a-46d3-b64b-fc8a9cf12de9
📒 Files selected for processing (19)
scripts/devenv-builder/configure-vm.shscripts/get-latest-rhocp-repo.shscripts/release-notes/gen_gh_releases_from_mirror.pyscripts/release-notes/gen_gh_releases_from_rhocp.pytest/assets/common_versions.sh.templatetest/bin/build_rpms.shtest/bin/common.shtest/bin/common_versions.shtest/bin/pyutils/build_bootc_images.pytest/bin/pyutils/generate_common_versions.pytest/bin/scenario.shtest/kickstart-templates/includes/post-cos9rpm.cfgtest/package-sources/rhocp-y.tomltest/package-sources/rhocp-y1.tomltest/package-sources/rhocp-y2.tomltest/scenarios/presubmits/el98-src@rpm-install.sh.disabledtest/scenarios/releases/el98@rpm-standard1.sh.disabledtest/scenarios/releases/el98@rpm-standard2.sh.disabledtest/scenarios/releases/el98@rpm-upgrade.sh.disabled
✅ Files skipped from review due to trivial changes (4)
- test/bin/common.sh
- test/bin/build_rpms.sh
- test/package-sources/rhocp-y2.toml
- test/package-sources/rhocp-y1.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- test/bin/scenario.sh
- test/package-sources/rhocp-y.toml
- test/scenarios/presubmits/el98-src@rpm-install.sh.disabled
- scripts/get-latest-rhocp-repo.sh
- test/scenarios/releases/el98@rpm-standard1.sh.disabled
- test/scenarios/releases/el98@rpm-standard2.sh.disabled
- test/scenarios/releases/el98@rpm-upgrade.sh.disabled
- scripts/release-notes/gen_gh_releases_from_rhocp.py
Contributor
Author
|
/test e2e-aws-tests |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
pacevedom
force-pushed
the
test-framework-5.0
branch
from
c6a53ad to
2db639e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/bin/pyutils/generate_common_versions.py (1)
275-285:⚠️ Potential issue | 🟠 MajorUnhandled
KeyErrorcan crash the script.
get_previous_version()raisesKeyErrorwhen stepping past defined majors, but this call (line 284) isn't wrapped intry/exceptlikeget_dependencies_repo_url(lines 113-116). If the loop exhaustsVERSION_MAP, the script crashes.Proposed fix
current_major, current_minor = major_version, minor_version for _ in range(4): for gitops_version_from_api_docs in data.get("data", [{}])[0].get("versions", []): gitops_version_ocp_compatibility = gitops_version_from_api_docs.get("openshift_compatibility") or "" gitops_version_number = gitops_version_from_api_docs.get("name") if f"{current_major}.{current_minor}" in gitops_version_ocp_compatibility: logging.info(f"Latest GitOps version: {gitops_version_number} which is compatible with OCP {gitops_version_ocp_compatibility}") return gitops_version_number - current_major, current_minor = get_previous_version(current_major, current_minor) + try: + current_major, current_minor = get_previous_version(current_major, current_minor) + except KeyError: + break return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` around lines 275 - 285, The loop in the version-selection logic can crash because get_previous_version(current_major, current_minor) may raise KeyError when versions are exhausted; wrap that call in a try/except KeyError inside the for-loop (the block using current_major/current_minor and gitops_version_from_api_docs) and on KeyError break out (or return an empty string) so the function safely falls through to the final return ""; ensure you update references to current_major/current_minor only when the call succeeds and preserve existing logging/return behavior for gitops_version_number.
🧹 Nitpick comments (1)
test/scenarios/presubmits/el98-src@rpm-install.sh.disabled (1)
44-72: Extractconfigure_rhocp_repo()into a shared helper.This exact repo-setup logic now lives here and in
test/scenarios/releases/el98@rpm-standard1.sh.disabledandtest/scenarios/releases/el98@rpm-standard2.sh.disabled. The next repo-format change will be easy to miss in one copy again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scenarios/presubmits/el98-src`@rpm-install.sh.disabled around lines 44 - 72, Extract the configure_rhocp_repo function into a shared helper script and replace the duplicated copies with calls to that helper: move the configure_rhocp_repo implementation (including use of run_command_on_vm, copy_file_to_vm, mktemp/tmp_file handling, ocp_repo_name logic and the two branches for numeric vs http rhocp values) into a common sourced file (e.g., a shared test helper) and update each caller script to source that helper and invoke configure_rhocp_repo with the same arguments; ensure the tmp file creation and removal, quoting, and error behavior are preserved when refactoring so existing behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 275-285: The loop in the version-selection logic can crash because
get_previous_version(current_major, current_minor) may raise KeyError when
versions are exhausted; wrap that call in a try/except KeyError inside the
for-loop (the block using current_major/current_minor and
gitops_version_from_api_docs) and on KeyError break out (or return an empty
string) so the function safely falls through to the final return ""; ensure you
update references to current_major/current_minor only when the call succeeds and
preserve existing logging/return behavior for gitops_version_number.
---
Nitpick comments:
In `@test/scenarios/presubmits/el98-src`@rpm-install.sh.disabled:
- Around line 44-72: Extract the configure_rhocp_repo function into a shared
helper script and replace the duplicated copies with calls to that helper: move
the configure_rhocp_repo implementation (including use of run_command_on_vm,
copy_file_to_vm, mktemp/tmp_file handling, ocp_repo_name logic and the two
branches for numeric vs http rhocp values) into a common sourced file (e.g., a
shared test helper) and update each caller script to source that helper and
invoke configure_rhocp_repo with the same arguments; ensure the tmp file
creation and removal, quoting, and error behavior are preserved when refactoring
so existing behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cfebe57e-2add-42e0-9763-426048363e77
📒 Files selected for processing (19)
scripts/devenv-builder/configure-vm.shscripts/get-latest-rhocp-repo.shscripts/release-notes/gen_gh_releases_from_mirror.pyscripts/release-notes/gen_gh_releases_from_rhocp.pytest/assets/common_versions.sh.templatetest/bin/build_rpms.shtest/bin/common.shtest/bin/common_versions.shtest/bin/pyutils/build_bootc_images.pytest/bin/pyutils/generate_common_versions.pytest/bin/scenario.shtest/kickstart-templates/includes/post-cos9rpm.cfgtest/package-sources/rhocp-y.tomltest/package-sources/rhocp-y1.tomltest/package-sources/rhocp-y2.tomltest/scenarios/presubmits/el98-src@rpm-install.sh.disabledtest/scenarios/releases/el98@rpm-standard1.sh.disabledtest/scenarios/releases/el98@rpm-standard2.sh.disabledtest/scenarios/releases/el98@rpm-upgrade.sh.disabled
✅ Files skipped from review due to trivial changes (5)
- test/bin/scenario.sh
- test/bin/common.sh
- scripts/release-notes/gen_gh_releases_from_rhocp.py
- test/package-sources/rhocp-y2.toml
- test/package-sources/rhocp-y1.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/get-latest-rhocp-repo.sh
- test/scenarios/releases/el98@rpm-upgrade.sh.disabled
- test/package-sources/rhocp-y.toml
- scripts/devenv-builder/configure-vm.sh
Contributor
|
@pacevedom: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.