./hack/post-release.sh omit bundle change#4184
Open
wgahnagl wants to merge 1 commit into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThe pre-release script now accepts a 🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
wgahnagl
changed the title
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jrvaldes, wgahnagl 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
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![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 `@hack/pre-release.sh`:
- Around line 442-449: The getopts option parsing lacks a fallback for invalid
flags, so update the case block that handles getopts (the "while getopts
\"bcdgk\" opt; do" and its case for "$opt") to add a default branch (*) that
prints a clear error/usage message and exits non‑zero; also handle the ":" case
if you want explicit "missing argument" detection for options that take
arguments. Ensure the new branch references the same variables (opt) and uses
the same script exit style as other failure paths so bad flags cause an
immediate non-zero exit instead of continuing execution.
- Around line 239-243: The script suppresses failures when reverting RBAC bundle
files by appending "2>/dev/null || true" to the git checkout command (the block
conditioned on KEEP_BUNDLE_CHANGES), so revert errors are hidden and the script
can continue with unintended RBAC changes; remove the suppression and ensure
failures are propagated by deleting "2>/dev/null || true" and/or adding explicit
error handling after the git checkout (check the exit status and exit with a
non-zero code or log and exit) for the git checkout of
bundle/manifests/prometheus-k8s_rbac.authorization.k8s.io_v1_role.yaml,
bundle/manifests/system-wicd-nodes_rbac.authorization.k8s.io_v1_clusterrole.yaml
and config/rbac/role.yaml so the script fails when the revert fails.
🪄 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: 5d3d0936-7be6-4265-bf8c-243796526487
📒 Files selected for processing (1)
hack/pre-release.sh
Comment on lines
+239
to
+243
| if [[ $KEEP_BUNDLE_CHANGES == "false" ]]; then | ||
| echo "Reverting RBAC changes from make bundle (use -k flag to keep all changes)" | ||
| git checkout HEAD -- bundle/manifests/prometheus-k8s_rbac.authorization.k8s.io_v1_role.yaml \ | ||
| bundle/manifests/system-wicd-nodes_rbac.authorization.k8s.io_v1_clusterrole.yaml \ | ||
| config/rbac/role.yaml 2>/dev/null || true |
There was a problem hiding this comment.
Do not suppress RBAC reversion failures.
At Line 243, 2>/dev/null || true hides checkout failures, so the script can continue while still carrying unintended RBAC changes (opposite of the default contract).
Suggested fix
- if [[ $KEEP_BUNDLE_CHANGES == "false" ]]; then
- echo "Reverting RBAC changes from make bundle (use -k flag to keep all changes)"
- git checkout HEAD -- bundle/manifests/prometheus-k8s_rbac.authorization.k8s.io_v1_role.yaml \
- bundle/manifests/system-wicd-nodes_rbac.authorization.k8s.io_v1_clusterrole.yaml \
- config/rbac/role.yaml 2>/dev/null || true
+ if [[ $KEEP_BUNDLE_CHANGES == "false" ]]; then
+ echo "Reverting RBAC changes from make bundle (use -k flag to keep all changes)"
+ for file in \
+ bundle/manifests/prometheus-k8s_rbac.authorization.k8s.io_v1_role.yaml \
+ bundle/manifests/system-wicd-nodes_rbac.authorization.k8s.io_v1_clusterrole.yaml \
+ config/rbac/role.yaml; do
+ if git ls-files --error-unmatch "$file" >/dev/null 2>&1; then
+ git checkout HEAD -- "$file"
+ fi
+ done
elseAs per coding guidelines, hack/**/* scripts must “Check for proper error handling.”
🤖 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/pre-release.sh` around lines 239 - 243, The script suppresses failures
when reverting RBAC bundle files by appending "2>/dev/null || true" to the git
checkout command (the block conditioned on KEEP_BUNDLE_CHANGES), so revert
errors are hidden and the script can continue with unintended RBAC changes;
remove the suppression and ensure failures are propagated by deleting
"2>/dev/null || true" and/or adding explicit error handling after the git
checkout (check the exit status and exit with a non-zero code or log and exit)
for the git checkout of
bundle/manifests/prometheus-k8s_rbac.authorization.k8s.io_v1_role.yaml,
bundle/manifests/system-wicd-nodes_rbac.authorization.k8s.io_v1_clusterrole.yaml
and config/rbac/role.yaml so the script fails when the revert fails.
Source: Coding guidelines
Comment on lines
+442
to
+449
| while getopts "bcdgk" opt; do | ||
| case "$opt" in | ||
| c) CVP=true;; | ||
| b) BASEIMAGE=true;; | ||
| d) DISTGIT=true;; | ||
| g) GITLAB=true;; | ||
| esac | ||
| k) KEEP_BUNDLE_CHANGES=true;; | ||
| esac |
There was a problem hiding this comment.
Handle unsupported flags explicitly in getopts.
There is no fallback branch for invalid options. At Line 443-449 this matches ShellCheck SC2220 and leads to unclear/partial execution on bad flags.
Suggested fix
while getopts "bcdgk" opt; do
case "$opt" in
c) CVP=true;;
b) BASEIMAGE=true;;
d) DISTGIT=true;;
g) GITLAB=true;;
k) KEEP_BUNDLE_CHANGES=true;;
+ \?)
+ echo "Invalid option: -$OPTARG"
+ usage
+ exit 2
+ ;;
esac
done As per coding guidelines, **/*.sh must “Validate shellcheck compliance.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while getopts "bcdgk" opt; do | |
| case "$opt" in | |
| c) CVP=true;; | |
| b) BASEIMAGE=true;; | |
| d) DISTGIT=true;; | |
| g) GITLAB=true;; | |
| esac | |
| k) KEEP_BUNDLE_CHANGES=true;; | |
| esac | |
| while getopts "bcdgk" opt; do | |
| case "$opt" in | |
| c) CVP=true;; | |
| b) BASEIMAGE=true;; | |
| d) DISTGIT=true;; | |
| g) GITLAB=true;; | |
| k) KEEP_BUNDLE_CHANGES=true;; | |
| \?) | |
| echo "Invalid option: -$OPTARG" | |
| usage | |
| exit 2 | |
| ;; | |
| esac | |
| done |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 443-449: Invalid flags are not handled. Add a *) case.
(SC2220)
🤖 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/pre-release.sh` around lines 442 - 449, The getopts option parsing lacks
a fallback for invalid flags, so update the case block that handles getopts (the
"while getopts \"bcdgk\" opt; do" and its case for "$opt") to add a default
branch (*) that prints a clear error/usage message and exits non‑zero; also
handle the ":" case if you want explicit "missing argument" detection for
options that take arguments. Ensure the new branch references the same variables
(opt) and uses the same script exit style as other failure paths so bad flags
cause an immediate non-zero exit instead of continuing execution.
Sources: Coding guidelines, Linters/SAST tools
Contributor
|
@wgahnagl: 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.
This makes it so that the bundle changes get reverted so they don't have to be reverted manually
Summary by CodeRabbit