NO-ISSUE: Fix get_latest_rhocp_repo to support crossing major versions#6519
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pacevedom: This pull request explicitly references no 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. |
WalkthroughThe PR refactors repository-version discovery in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
approved
![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
🧹 Nitpick comments (1)
scripts/get-latest-rhocp-repo.sh (1)
60-75:max_steps=1when args are provided makes the fallback logic unreachable for those callers.With explicit
major/minorargs,get_prev_versionat lines 95-100 is computed but never consumed (loop exits after one iteration). Harmless, just dead work — you could early-return after the two repo probes in that branch, or accept it as-is for simplicity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/get-latest-rhocp-repo.sh` around lines 60 - 75, The branch that sets current_minor/current_major when arguments are provided still sets max_steps=1 causing the loop that uses get_prev_version to exit after one iteration so its computation is dead work; fix by either returning early from the "both minor and major provided" and "only minor provided" branches after the two repo probes (so get_prev_version is never invoked for callers that passed args) or change max_steps in those branches to the intended value (e.g., the same default as the no-args branch) so the fallback loop can run; update the logic around current_minor, current_major, max_steps and the call sites that rely on get_prev_version to ensure either an early return or correct max_steps is used.
🤖 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/get-latest-rhocp-repo.sh`:
- Around line 26-27: The hardcoded associative array LAST_MINOR_FOR_MAJOR
(declare -A LAST_MINOR_FOR_MAJOR=([4]=22)) can go stale; update the script to
either derive the last minor dynamically or add a clear TODO comment for
maintainers to update it when a new 4.x minor ships. Locate the
LAST_MINOR_FOR_MAJOR declaration in scripts/get-latest-rhocp-repo.sh and replace
or augment it: preferably implement logic to compute the highest 4.z available
(e.g., by listing repo tags/releases and parsing the max minor) or, if not
feasible, add a brief comment next to LAST_MINOR_FOR_MAJOR noting it must be
bumped when 4.x publishes a newer minor and include the date/owner for
accountability.
---
Nitpick comments:
In `@scripts/get-latest-rhocp-repo.sh`:
- Around line 60-75: The branch that sets current_minor/current_major when
arguments are provided still sets max_steps=1 causing the loop that uses
get_prev_version to exit after one iteration so its computation is dead work;
fix by either returning early from the "both minor and major provided" and "only
minor provided" branches after the two repo probes (so get_prev_version is never
invoked for callers that passed args) or change max_steps in those branches to
the intended value (e.g., the same default as the no-args branch) so the
fallback loop can run; update the logic around current_minor, current_major,
max_steps and the call sites that rely on get_prev_version to ensure either an
early return or correct max_steps is used.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b683f1a-e75f-4550-95e4-c986fd1920ae
📒 Files selected for processing (1)
scripts/get-latest-rhocp-repo.sh
|
/test ? |
|
/test ocp-full-conformance-rhel-eus |
|
/lgtm |
pacevedom
force-pushed
the
configure-vm-5.0
branch
from
42a8239 to
48fab8c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/devenv-builder/configure-vm.sh (1)
217-218:⚠️ Potential issue | 🔴 CriticalLine 218 numeric branch has asymmetric output handling—only receives minor, not major.
get-latest-rhocp-repo.shoutputs only${check_minor}on line 85 (numeric path) but${rhocp_beta_url},${check_major},${check_minor}on line 91 (beta path). When the helper crosses major boundaries and finds a hit in the numeric path, configure-vm.sh applies a minor from a different major to the hardcodedocp_majorfrom Makefile. The beta branch avoids this by parsing both; align the numeric path to also include and parse the major (e.g., change line 85 toecho "${check_major},${check_minor}"and update the parser in configure-vm.sh accordingly).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/devenv-builder/configure-vm.sh` around lines 217 - 218, The numeric path of the helper only returns the minor, causing configure-vm.sh to combine a parsed minor with the hardcoded ocp_major; update get-latest-rhocp-repo.sh numeric branch to output both check_major and check_minor (e.g., echo "${check_major},${check_minor}") and then modify configure-vm.sh where it reads RHOCP/parsed values so it splits the helper output into parsed_major and parsed_minor and uses parsed_major instead of the hardcoded ocp_major when constructing the repo string in the subscription-manager repos --enable "rhocp-${parsed_major}.${parsed_minor}-for-rhel-9-$(uname -m)-rpms" branch (keep the existing beta parsing behavior for the other branch).
🧹 Nitpick comments (1)
scripts/devenv-builder/configure-vm.sh (1)
199-214: Nit: duplicatedget_prev_version/LAST_MINOR_FOR_MAJORwithget-latest-rhocp-repo.sh.Same logic now lives in two scripts. Since
configure-vm.shalready sources${RHOCP_REPO}via execution, consider exposing the helper from that script (or a small shared lib) to keep the cross-major map in one place. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/devenv-builder/configure-vm.sh` around lines 199 - 214, Duplicate get_prev_version and last_minor_for_major logic should be centralized; remove the copy from configure-vm.sh and consume the canonical implementation from get-latest-rhocp-repo.sh (or a new small shared lib). Export or move the map (last_minor_for_major / LAST_MINOR_FOR_MAJOR) and the function get_prev_version into the repo script (or shared lib) and ensure configure-vm.sh sources RHOCP_REPO (or the shared lib) and calls get_prev_version rather than defining its own copy; update any references to prev_major/prev_minor to use the centralized helper names so there are no conflicting local definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/devenv-builder/configure-vm.sh`:
- Around line 217-218: The numeric path of the helper only returns the minor,
causing configure-vm.sh to combine a parsed minor with the hardcoded ocp_major;
update get-latest-rhocp-repo.sh numeric branch to output both check_major and
check_minor (e.g., echo "${check_major},${check_minor}") and then modify
configure-vm.sh where it reads RHOCP/parsed values so it splits the helper
output into parsed_major and parsed_minor and uses parsed_major instead of the
hardcoded ocp_major when constructing the repo string in the
subscription-manager repos --enable
"rhocp-${parsed_major}.${parsed_minor}-for-rhel-9-$(uname -m)-rpms" branch (keep
the existing beta parsing behavior for the other branch).
---
Nitpick comments:
In `@scripts/devenv-builder/configure-vm.sh`:
- Around line 199-214: Duplicate get_prev_version and last_minor_for_major logic
should be centralized; remove the copy from configure-vm.sh and consume the
canonical implementation from get-latest-rhocp-repo.sh (or a new small shared
lib). Export or move the map (last_minor_for_major / LAST_MINOR_FOR_MAJOR) and
the function get_prev_version into the repo script (or shared lib) and ensure
configure-vm.sh sources RHOCP_REPO (or the shared lib) and calls
get_prev_version rather than defining its own copy; update any references to
prev_major/prev_minor to use the centralized helper names so there are no
conflicting local definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 799812aa-8eb0-4f33-8a1b-3e542ad80ba7
📒 Files selected for processing (2)
scripts/devenv-builder/configure-vm.shscripts/get-latest-rhocp-repo.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/get-latest-rhocp-repo.sh
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom, pmtk 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 |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@pacevedom: 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. |
3 participants
Summary by CodeRabbit