USHIFT-6588: Improve usability of generate_common_versions.py script#6663
Conversation
|
@vanhalenar: This pull request references USHIFT-6588 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 "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-robot
added
the
jira/valid-reference
|
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 ChangesVersion management CLI + build integration
sequenceDiagram
participant CI as CI job / ci_phase_iso_build.sh
participant CLI as manage_common_versions.sh
participant Git as Git (diff)
participant Venv as create-venv.sh / Python venv
participant Gen as generate_common_versions.py
CI->>CLI: run "verify"
CLI->>Git: git diff ${SCENARIO_BUILD_BRANCH}^1...HEAD check common_versions.sh
alt diff found
CLI->>Venv: create venv
Venv->>Gen: run generator -> write common_versions.sh
Gen->>Git: compare regenerated file
alt still differs
CLI-->>CI: exit non-zero (failure)
else
CLI-->>CI: success, continue
end
else
CLI-->>CI: success, continue
end
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 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 `@test/bin/manage_common_versions.sh`:
- Around line 10-13: The git diff invocation is using an unnecessary parent
traversal token (${SCENARIO_BUILD_BRANCH}^1...HEAD) which can misdetect changes;
update the diff command that checks common_versions.sh (the line invoking git
diff --exit-code with ${SCENARIO_BUILD_BRANCH}^1...HEAD and
${ROOTDIR}/test/bin/common_versions.sh) to use ${SCENARIO_BUILD_BRANCH}...HEAD
instead so the three-dot range uses the proper merge-base and accurately detects
PR changes.
🪄 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: Enterprise
Run ID: 5398172a-090f-4fd2-806f-f6f9acdbfb4a
📒 Files selected for processing (2)
test/bin/ci_phase_iso_build.shtest/bin/manage_common_versions.sh
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 `@test/bin/ci_phase_iso_build.sh`:
- Around line 184-186: Update the stale ShellCheck source comment to match the
actual sourced script: replace the SC1091 directive that references
common_versions_verify.sh with one that references manage_common_versions.sh
(the file actually sourced via source "${SCRIPTDIR}/manage_common_versions.sh"
verify) so ShellCheck correctly recognizes the sourced file and stops reporting
SC1091; ensure the comment references the same SCRIPTDIR usage as the source
line.
🪄 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: Enterprise
Run ID: 38201f04-d793-4ab5-bc94-674d99a6db5a
📒 Files selected for processing (1)
test/bin/ci_phase_iso_build.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/bin/ci_phase_iso_build.sh (1)
184-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a ShellCheck source directive for
manage_common_versions.sh.Line 184 still triggers SC1091 without an explicit source hint.
Suggested fix
+# shellcheck source=test/bin/manage_common_versions.sh source "${SCRIPTDIR}/manage_common_versions.sh" verify🤖 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 `@test/bin/ci_phase_iso_build.sh` at line 184, Add a ShellCheck source directive above the line that sources manage_common_versions.sh so SC1091 is silenced: insert a comment like a shellcheck source hint referencing manage_common_versions.sh immediately before the source "${SCRIPTDIR}/manage_common_versions.sh" verify line (the line that calls the script), ensuring ShellCheck can resolve the sourced file.
🤖 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 `@test/bin/ci_phase_iso_build.sh`:
- Line 184: Add a ShellCheck source directive above the line that sources
manage_common_versions.sh so SC1091 is silenced: insert a comment like a
shellcheck source hint referencing manage_common_versions.sh immediately before
the source "${SCRIPTDIR}/manage_common_versions.sh" verify line (the line that
calls the script), ensuring ShellCheck can resolve the sourced file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a2445143-641b-4a58-aa19-168b58dea41c
📒 Files selected for processing (1)
test/bin/ci_phase_iso_build.sh
vanhalenar
changed the title
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggiguash, vanhalenar 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
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@vanhalenar: 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. |
|
Not related to the fix - saving CI cycles. |
|
@ggiguash: Overrode contexts on behalf of ggiguash: ci/prow/e2e-aws-tests-bootc-periodic-el10 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. |
|
@vanhalenar: 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
This PR add the
manage_common_versions.shscript, which has two actions,generateandverify. The script takes care of creating the venv forgenerate_common_versions.pyand replaces thecommon_versions_verify.shscript for verification. This is more in line with other scripts we have, e.g.manage_webserver.sh.Summary by CodeRabbit