NO-JIRA: Script to automate rebase #309
Open
Conversation
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
openshift-ci-robot
added
the
jira/valid-reference
|
@PillaiManish: This pull request explicitly references no jira issue. In 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. |
WalkthroughAdds a new Bash script rebase_automation.sh that automates a multi-step rebase process: checks prerequisites, detects versions, optionally creates a backup branch, parses flags (dry-run, single step, skip-commit), and executes steps to bump Go deps, update Makefile, update bundle/CSV files, perform repo-wide replacements, and optionally commit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant S as rebase_automation.sh
participant G as git
participant T as Tools (go/make/sed/grep)
participant FS as Filesystem
U->>S: Run script with flags (-d, -s N, --skip-commit)
S->>S: Parse args / flags
S->>T: Check prerequisites
T-->>S: Tools present?
S->>FS: Read Makefile to detect OLD_* (if needed)
alt not dry-run
S->>G: Create backup branch
G-->>S: Branch created
end
alt Step=1 or all
S->>T: go get / mod tidy / vendor
S->>G: Optional commit (Step 1)
end
alt Step=2 or all
S->>FS: Update Makefile vars
S->>T: make update && make bundle
S->>G: Optional commit (Step 2)
end
alt Step=3 or all
S->>FS: Update CSV/bundle files, bundle.Dockerfile
S->>T: make update-bindata
S->>G: Optional commit (Step 3)
end
alt Step=4 or all
S->>FS: Safe replacements across repo
S->>T: make manifests bundle
S->>G: Commit if changes (Step 4)
end
S-->>U: Summary / next steps (dry-run prints plan)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: PillaiManish The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
rebase_automation.sh (6)
14-15: Remove or use unused REPO_ROOT.
REPO_ROOT="$SCRIPT_DIR"is never used.Apply this diff:
-REPO_ROOT="$SCRIPT_DIR"
76-109: Prereqs: include awk; clarify sed portability in help.Since detection will be more robust with awk (see next comment), add
awkto required tools. Also mention BSD/GNU sed portability in docs if you don't adopt the wrapper.Apply this diff:
- local required_tools=("go" "make" "sed" "grep") + local required_tools=("go" "make" "sed" "grep" "awk")
140-146: Avoid SC2155; split declaration/assignment and guard branch collision.Creating the branch name inline masks errors; also fail if branch already exists.
Apply this diff:
- local backup_branch="backup-$(date +%Y%m%d-%H%M%S)" - git branch "$backup_branch" + local backup_branch + backup_branch="backup-$(date +%Y%m%d-%H%M%S)" + if git rev-parse --verify --quiet "$backup_branch" >/dev/null; then + log_warning "Backup branch $backup_branch already exists; generating a new suffix." + backup_branch="backup-$(date +%Y%m%d-%H%M%S)-$$" + fi + git branch "$backup_branch"
416-419: Run make targets separately for clearer failure reporting (optional).Two separate invocations make it easier to see which target failed.
Apply this diff:
- log_info "Running: make manifests bundle" - make manifests bundle + log_info "Running: make manifests" + make manifests + log_info "Running: make bundle" + make bundle
219-224: Limit git add scope (optional).
git add .may capture unintended files. Prefer targeted paths.Example:
- git add . + git add Makefile config/manifests/bases/ bundle/ bundle.Dockerfile
1-527: Fix ShellCheck warnings in rebase_automation.sh
- Remove or export the unused variable
REPO_ROOT(SC2034, line 14).- Split declaration and assignment for locals to avoid masking return values (SC2155) at lines 143, 197–198, 272–273, 280–281, 335.
- Double-quote variable expansions in
echoto prevent globbing/word-splitting (SC2086) at line 191.- Replace command-substitution-based array splits with
mapfileorread -ato avoid splitting pitfalls (SC2207) at lines 315 and 407.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
rebase_automation.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
rebase_automation.sh
[warning] 14-14: REPO_ROOT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 143-143: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 197-197: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 198-198: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 272-272: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 273-273: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 280-280: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 281-281: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 315-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 335-335: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 407-407: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🔇 Additional comments (3)
rebase_automation.sh (3)
1-3: Good baseline: strict shell flags enabled.
set -euo pipefailis appropriate for this automation script.
392-402: Guard against double replacement in bundle version loop using exact boundaries.After fixing boundaries above, this loop logic is fine. No change required beyond adopting
safe_replace_version.
486-524: Overall flow looks solid.Good separation of concerns, dry-run, and per-step execution.
Comment on lines
+39
to
+75
| # Function to display usage | ||
| usage() { | ||
| cat << EOF | ||
| Usage: $0 [OPTIONS] | ||
|
|
||
| Automates the cert-manager-operator rebase process. | ||
|
|
||
| Environment Variables: | ||
| NEW_CERT_MANAGER_VERSION New cert-manager version (e.g., 1.18.2) | ||
| NEW_BUNDLE_VERSION New bundle version (e.g., 1.18.0) | ||
| OLD_BUNDLE_VERSION Old bundle version to replace (optional - auto-detected) | ||
| OLD_CERT_MANAGER_VERSION Old cert-manager version to replace (optional - auto-detected) | ||
|
|
||
| Options: | ||
| -h, --help Show this help message | ||
| -d, --dry-run Show what would be done without making changes | ||
| -s, --step STEP Run only specific step (1-4) | ||
| --skip-commit Skip git commits (useful for testing) | ||
|
|
||
| Examples: | ||
| # Basic usage with environment variables | ||
| NEW_CERT_MANAGER_VERSION=1.19.0 NEW_BUNDLE_VERSION=1.19.0 $0 | ||
|
|
||
| # Dry run to see what would be changed | ||
| NEW_CERT_MANAGER_VERSION=1.19.0 NEW_BUNDLE_VERSION=1.19.0 $0 --dry-run | ||
|
|
||
| # Run only step 2 | ||
| NEW_CERT_MANAGER_VERSION=1.19.0 NEW_BUNDLE_VERSION=1.19.0 $0 --step 2 | ||
|
|
||
| Steps: | ||
| 1. Bump deps with upstream cert-manager | ||
| 2. Update Makefile: BUNDLE_VERSION, CERT_MANAGER_VERSION, CHANNELS | ||
| 3. Update CSV: OLM bundle name, version, replaces, skipRange | ||
| 4. More manual replacements | ||
| EOF | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add portable sed wrapper and escaping helpers (BSD/GNU compatibility).
You rely on sed -i and BRE features (\+) in many places. On macOS/BSD sed this fails. Introduce helpers and use -E consistently.
Apply this diff (insert after log_error):
+# --- sed helpers (GNU/BSD portable) ---
+sed_inplace() {
+ # usage: sed_inplace 's|pattern|replacement|g' file
+ if sed --version >/dev/null 2>&1; then
+ sed -i -E "$1" "$2"
+ else
+ sed -i '' -E "$1" "$2"
+ fi
+}
+
+# Escape text for use in sed regex pattern and replacement
+escape_sed_pat() { printf '%s' "$1" | sed -e 's/[.[\*^$()+?{}|\\]/\\&/g' -e 's|/|\\/|g'; }
+escape_sed_repl() { printf '%s' "$1" | sed -e 's/[&\\]/\\&/g'; }🤖 Prompt for AI Agents
In rebase_automation.sh around lines 39 to 75, add a portable sed wrapper and
escaping helper functions immediately after the existing log_error function:
create a sed_inplace function that detects BSD (macOS) vs GNU sed and invokes
sed -i '' for BSD or sed -i for GNU, a sed_regex_escape helper to escape input
when building regex patterns, and a sed_replacement_escape helper to escape
replacement strings (handling & and backreferences); update all sed calls to use
sed_inplace and switch to extended regex (-E) instead of BREs (replace
occurrences of \+ with + or use + inside -E), and use the escape helpers when
interpolating variables into sed expressions so the script works on both
macOS/BSD and GNU sed.
Comment on lines
+116
to
+125
| if [[ -z "$OLD_BUNDLE_VERSION" ]]; then | ||
| OLD_BUNDLE_VERSION=$(grep "^BUNDLE_VERSION" Makefile | cut -d'=' -f2 | tr -d ' ?') | ||
| log_info "Auto-detected OLD_BUNDLE_VERSION: $OLD_BUNDLE_VERSION" | ||
| fi | ||
|
|
||
| # Extract current cert-manager version from Makefile | ||
| if [[ -z "$OLD_CERT_MANAGER_VERSION" ]]; then | ||
| OLD_CERT_MANAGER_VERSION=$(grep "^CERT_MANAGER_VERSION" Makefile | cut -d'=' -f2 | tr -d ' ?"v') | ||
| log_info "Auto-detected OLD_CERT_MANAGER_VERSION: $OLD_CERT_MANAGER_VERSION" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make version auto-detection robust to spacing/quotes.
Current grep/cut/tr is brittle. Use awk to trim and strip quotes/"v".
Apply this diff:
- OLD_BUNDLE_VERSION=$(grep "^BUNDLE_VERSION" Makefile | cut -d'=' -f2 | tr -d ' ?')
+ OLD_BUNDLE_VERSION=$(awk -F'=' '/^BUNDLE_VERSION[[:space:]]*\?/ {gsub(/[[:space:]]*/,"",$1); v=$2; gsub(/[[:space:]]*/,"",v); gsub(/"/,"",v); print v}' Makefile)
log_info "Auto-detected OLD_BUNDLE_VERSION: $OLD_BUNDLE_VERSION"
@@
- OLD_CERT_MANAGER_VERSION=$(grep "^CERT_MANAGER_VERSION" Makefile | cut -d'=' -f2 | tr -d ' ?"v')
+ OLD_CERT_MANAGER_VERSION=$(awk -F'=' '/^CERT_MANAGER_VERSION[[:space:]]*\?/ {v=$2; gsub(/[[:space:]]*/,"",v); gsub(/"/,"",v); gsub(/^v/,"",v); print v}' Makefile)
log_info "Auto-detected OLD_CERT_MANAGER_VERSION: $OLD_CERT_MANAGER_VERSION"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 116-125, the current grep/cut/tr pipeline
for auto-detecting BUNDLE_VERSION and CERT_MANAGER_VERSION is brittle; replace
it with an awk one-liner that matches the variable name, splits on '=', trims
leading/trailing whitespace from the value, strips surrounding single/double
quotes and any leading lowercase 'v' characters, and prints the cleaned value
for assignment; keep the existing log_info calls unchanged so the script still
logs the detected versions.
Comment on lines
+196
to
+211
| # Extract major.minor versions for channels | ||
| local old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2) | ||
| local new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2) | ||
|
|
||
| # Update BUNDLE_VERSION | ||
| log_info "Updating BUNDLE_VERSION: $OLD_BUNDLE_VERSION -> $NEW_BUNDLE_VERSION" | ||
| sed -i "s/^BUNDLE_VERSION ?= $OLD_BUNDLE_VERSION/BUNDLE_VERSION ?= $NEW_BUNDLE_VERSION/" Makefile | ||
|
|
||
| # Update CERT_MANAGER_VERSION | ||
| log_info "Updating CERT_MANAGER_VERSION: v$OLD_CERT_MANAGER_VERSION -> v$NEW_CERT_MANAGER_VERSION" | ||
| sed -i "s/^CERT_MANAGER_VERSION ?= \"v$OLD_CERT_MANAGER_VERSION\"/CERT_MANAGER_VERSION ?= \"v$NEW_CERT_MANAGER_VERSION\"/" Makefile | ||
|
|
||
| # Update CHANNELS | ||
| log_info "Updating CHANNELS: stable-v1,stable-v$old_channel_version -> stable-v1,stable-v$new_channel_version" | ||
| sed -i "s/^CHANNELS ?= \"stable-v1,stable-v$old_channel_version\"/CHANNELS ?= \"stable-v1,stable-v$new_channel_version\"/" Makefile | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use sed_inplace + escaped versions for Makefile edits.
Raw variables inside sed can break on special chars and BSD sed. Also fix SC2155 (split locals).
Apply this diff:
- local old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2)
- local new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2)
+ local old_channel_version
+ old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2)
+ local new_channel_version
+ new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2)
+ local obv_pat nbv_repl ocv_pat ncv_repl
+ obv_pat=$(escape_sed_pat "$OLD_BUNDLE_VERSION"); nbv_repl=$(escape_sed_repl "$NEW_BUNDLE_VERSION")
+ ocv_pat=$(escape_sed_pat "$OLD_CERT_MANAGER_VERSION"); ncv_repl=$(escape_sed_repl "$NEW_CERT_MANAGER_VERSION")
@@
- sed -i "s/^BUNDLE_VERSION ?= $OLD_BUNDLE_VERSION/BUNDLE_VERSION ?= $NEW_BUNDLE_VERSION/" Makefile
+ sed_inplace "s|^BUNDLE_VERSION[[:space:]]*\\?=[[:space:]]*$obv_pat|BUNDLE_VERSION ?= $nbv_repl|" Makefile
@@
- sed -i "s/^CERT_MANAGER_VERSION ?= \"v$OLD_CERT_MANAGER_VERSION\"/CERT_MANAGER_VERSION ?= \"v$NEW_CERT_MANAGER_VERSION\"/" Makefile
+ sed_inplace "s|^CERT_MANAGER_VERSION[[:space:]]*\\?=[[:space:]]*\"v$ocv_pat\"|CERT_MANAGER_VERSION ?= \"v$ncv_repl\"|" Makefile
@@
- sed -i "s/^CHANNELS ?= \"stable-v1,stable-v$old_channel_version\"/CHANNELS ?= \"stable-v1,stable-v$new_channel_version\"/" Makefile
+ sed_inplace "s|^CHANNELS[[:space:]]*\\?=[[:space:]]*\"stable-v1,stable-v$old_channel_version\"|CHANNELS ?= \"stable-v1,stable-v$new_channel_version\"|" Makefile📝 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
| # Extract major.minor versions for channels | |
| local old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2) | |
| local new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2) | |
| # Update BUNDLE_VERSION | |
| log_info "Updating BUNDLE_VERSION: $OLD_BUNDLE_VERSION -> $NEW_BUNDLE_VERSION" | |
| sed -i "s/^BUNDLE_VERSION ?= $OLD_BUNDLE_VERSION/BUNDLE_VERSION ?= $NEW_BUNDLE_VERSION/" Makefile | |
| # Update CERT_MANAGER_VERSION | |
| log_info "Updating CERT_MANAGER_VERSION: v$OLD_CERT_MANAGER_VERSION -> v$NEW_CERT_MANAGER_VERSION" | |
| sed -i "s/^CERT_MANAGER_VERSION ?= \"v$OLD_CERT_MANAGER_VERSION\"/CERT_MANAGER_VERSION ?= \"v$NEW_CERT_MANAGER_VERSION\"/" Makefile | |
| # Update CHANNELS | |
| log_info "Updating CHANNELS: stable-v1,stable-v$old_channel_version -> stable-v1,stable-v$new_channel_version" | |
| sed -i "s/^CHANNELS ?= \"stable-v1,stable-v$old_channel_version\"/CHANNELS ?= \"stable-v1,stable-v$new_channel_version\"/" Makefile | |
| # Extract major.minor versions for channels | |
| local old_channel_version | |
| old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2) | |
| local new_channel_version | |
| new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2) | |
| local obv_pat nbv_repl ocv_pat ncv_repl | |
| obv_pat=$(escape_sed_pat "$OLD_BUNDLE_VERSION"); nbv_repl=$(escape_sed_repl "$NEW_BUNDLE_VERSION") | |
| ocv_pat=$(escape_sed_pat "$OLD_CERT_MANAGER_VERSION"); ncv_repl=$(escape_sed_repl "$NEW_CERT_MANAGER_VERSION") | |
| # Update BUNDLE_VERSION | |
| log_info "Updating BUNDLE_VERSION: $OLD_BUNDLE_VERSION -> $NEW_BUNDLE_VERSION" | |
| sed_inplace "s|^BUNDLE_VERSION[[:space:]]*\\?=[[:space:]]*$obv_pat|BUNDLE_VERSION ?= $nbv_repl|" Makefile | |
| # Update CERT_MANAGER_VERSION | |
| log_info "Updating CERT_MANAGER_VERSION: v$OLD_CERT_MANAGER_VERSION -> v$NEW_CERT_MANAGER_VERSION" | |
| sed_inplace "s|^CERT_MANAGER_VERSION[[:space:]]*\\?=[[:space:]]*\"v$ocv_pat\"|CERT_MANAGER_VERSION ?= \"v$ncv_repl\"|" Makefile | |
| # Update CHANNELS | |
| log_info "Updating CHANNELS: stable-v1,stable-v$old_channel_version -> stable-v1,stable-v$new_channel_version" | |
| sed_inplace "s|^CHANNELS[[:space:]]*\\?=[[:space:]]*\"stable-v1,stable-v$old_channel_version\"|CHANNELS ?= \"stable-v1,stable-v$new_channel_version\"|" Makefile |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 197-197: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 198-198: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 196-211: the current sed calls embed raw
variables (which can break on special chars and BSD sed) and declare locals with
command substitutions triggering SC2155; replace inline sed -i usages with the
project's sed_inplace helper and pass escaped variable values (use the existing
sed_escape function) for OLD_BUNDLE_VERSION, NEW_BUNDLE_VERSION,
OLD_CERT_MANAGER_VERSION, NEW_CERT_MANAGER_VERSION and the channel version vars;
also avoid SC2155 by declaring locals separately (declare the variable name
first, then assign with the command substitution on the next line) when
computing old_channel_version and new_channel_version. Ensure the sed patterns
use the escaped variables and the sed_inplace helper for portability.
Comment on lines
+231
to
+297
| log_info "Step 3: Update CSV: OLM bundle name, version, replaces, skipRange and skips" | ||
|
|
||
| if [[ "$DRY_RUN" == "true" ]]; then | ||
| log_warning "[DRY RUN] Would update CSV files:" | ||
| echo " version: $OLD_BUNDLE_VERSION -> $NEW_BUNDLE_VERSION" | ||
| echo " name: cert-manager-operator.v$OLD_BUNDLE_VERSION -> cert-manager-operator.v$NEW_BUNDLE_VERSION" | ||
| echo " replaces: cert-manager-operator.v[previous] -> cert-manager-operator.v$OLD_BUNDLE_VERSION" | ||
| echo " skipRange: >=1.17.0 <1.18.0 -> >=$OLD_BUNDLE_VERSION <$NEW_BUNDLE_VERSION" | ||
| echo " Would run: make update-bindata" | ||
| return 0 | ||
| fi | ||
|
|
||
| # Files to update | ||
| local csv_files=( | ||
| "config/manifests/bases/cert-manager-operator.clusterserviceversion.yaml" | ||
| "bundle/manifests/cert-manager-operator.clusterserviceversion.yaml" | ||
| ) | ||
|
|
||
| for csv_file in "${csv_files[@]}"; do | ||
| if [[ -f "$csv_file" ]]; then | ||
| log_info "Updating $csv_file" | ||
|
|
||
| # Update version | ||
| sed -i "s/version: $OLD_BUNDLE_VERSION/version: $NEW_BUNDLE_VERSION/" "$csv_file" | ||
|
|
||
| # Update name | ||
| sed -i "s/name: cert-manager-operator.v$OLD_BUNDLE_VERSION/name: cert-manager-operator.v$NEW_BUNDLE_VERSION/" "$csv_file" | ||
|
|
||
| # Update replaces (should point to the old version that we're replacing) | ||
| sed -i "s/replaces: cert-manager-operator\.v[0-9]\+\.[0-9]\+\.[0-9]\+/replaces: cert-manager-operator.v$OLD_BUNDLE_VERSION/" "$csv_file" | ||
|
|
||
| # Update skipRange | ||
| sed -i "s/olm.skipRange: '>=.*<.*'/olm.skipRange: '>=$OLD_BUNDLE_VERSION <$NEW_BUNDLE_VERSION'/" "$csv_file" | ||
|
|
||
| # Note: Description updates will be handled in Step 4 (manual replacements) | ||
| fi | ||
| done | ||
|
|
||
| # Update bundle.Dockerfile | ||
| if [[ -f "bundle.Dockerfile" ]]; then | ||
| log_info "Updating bundle.Dockerfile" | ||
| local old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2) | ||
| local new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2) | ||
| sed -i "s/stable-v1,stable-v$old_channel_version/stable-v1,stable-v$new_channel_version/" bundle.Dockerfile | ||
| fi | ||
|
|
||
| # Update bundle metadata | ||
| if [[ -f "bundle/metadata/annotations.yaml" ]]; then | ||
| log_info "Updating bundle/metadata/annotations.yaml" | ||
| local old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2) | ||
| local new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2) | ||
| sed -i "s/stable-v1,stable-v$old_channel_version/stable-v1,stable-v$new_channel_version/" bundle/metadata/annotations.yaml | ||
| fi | ||
|
|
||
| # Run make update-bindata | ||
| log_info "Running: make update-bindata" | ||
| make update-bindata | ||
|
|
||
| # Commit changes | ||
| if [[ "$SKIP_COMMIT" != "true" ]]; then | ||
| git add . | ||
| git commit -m "Update CSV: OLM bundle name, version, replaces, skipRange and skips" | ||
| log_success "Step 3 committed" | ||
| fi | ||
|
|
||
| log_success "Step 3 completed" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix sed portability and regex in CSV updates.
Use sed_inplace and -E with + quantifiers. Split SC2155 locals.
Apply this diff:
- sed -i "s/version: $OLD_BUNDLE_VERSION/version: $NEW_BUNDLE_VERSION/" "$csv_file"
+ sed_inplace "s|^version:[[:space:]]*$(
+ escape_sed_pat "$OLD_BUNDLE_VERSION"
+ )$|version: $(
+ escape_sed_repl "$NEW_BUNDLE_VERSION"
+ )|" "$csv_file"
@@
- sed -i "s/name: cert-manager-operator.v$OLD_BUNDLE_VERSION/name: cert-manager-operator.v$NEW_BUNDLE_VERSION/" "$csv_file"
+ sed_inplace "s|^name:[[:space:]]*cert-manager-operator.v$(
+ escape_sed_pat "$OLD_BUNDLE_VERSION"
+ )$|name: cert-manager-operator.v$(
+ escape_sed_repl "$NEW_BUNDLE_VERSION"
+ )|" "$csv_file"
@@
- sed -i "s/replaces: cert-manager-operator\.v[0-9]\+\.[0-9]\+\.[0-9]\+/replaces: cert-manager-operator.v$OLD_BUNDLE_VERSION/" "$csv_file"
+ sed_inplace "s|replaces:[[:space:]]*cert-manager-operator\.v[0-9]+\.[0-9]+\.[0-9]+|replaces: cert-manager-operator.v$(
+ escape_sed_repl "$OLD_BUNDLE_VERSION"
+ )|" "$csv_file"
@@
- sed -i "s/olm.skipRange: '>=.*<.*'/olm.skipRange: '>=$OLD_BUNDLE_VERSION <$NEW_BUNDLE_VERSION'/" "$csv_file"
+ sed_inplace "s|olm\.skipRange:[[:space:]]*'>=[^<]++<[^']+'|olm.skipRange: '>=$(
+ escape_sed_repl "$OLD_BUNDLE_VERSION"
+ ) <$(escape_sed_repl "$NEW_BUNDLE_VERSION")'|" "$csv_file"
@@
- local old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2)
- local new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2)
- sed -i "s/stable-v1,stable-v$old_channel_version/stable-v1,stable-v$new_channel_version/" bundle.Dockerfile
+ local old_channel_version
+ old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2)
+ local new_channel_version
+ new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2)
+ sed_inplace "s|stable-v1,stable-v$old_channel_version|stable-v1,stable-v$new_channel_version|" bundle.Dockerfile
@@
- local old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2)
- local new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2)
- sed -i "s/stable-v1,stable-v$old_channel_version/stable-v1,stable-v$new_channel_version/" bundle/metadata/annotations.yaml
+ local old_channel_version
+ old_channel_version=$(echo "$OLD_BUNDLE_VERSION" | cut -d'.' -f1,2)
+ local new_channel_version
+ new_channel_version=$(echo "$NEW_BUNDLE_VERSION" | cut -d'.' -f1,2)
+ sed_inplace "s|stable-v1,stable-v$old_channel_version|stable-v1,stable-v$new_channel_version|" bundle/metadata/annotations.yamlCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 272-272: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 273-273: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 280-280: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 281-281: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 231-297, the sed invocations are not
portable and use basic regex plus in-place flags inconsistently, and some locals
are declared with command substitution on the same line (SC2155). Replace sed -i
calls with the helper sed_inplace function (which handles BSD/GNU differences),
add -E to enable extended regex and use + quantifiers (e.g. [0-9]+), and quote
variables inside the sed expressions; also split statements like local
old_channel_version=$(...) into two lines (declare local then assign) to satisfy
SC2155 and improve readability. Ensure all updated sed patterns use sed_inplace
-E "s/.../.../" and update skipRange pattern to use the proper variable
interpolation with quoted values.
Comment on lines
+314
to
+321
| local files_to_check=( | ||
| $(find . -type f \( -name "*.go" -o -name "*.yaml" -o -name "*.yml" -o -name "*.json" -o -name "*.md" -o -name "*.Dockerfile" \) \ | ||
| -not -path "./vendor/*" \ | ||
| -not -path "./.git/*" \ | ||
| -not -path "./testbin/*" \ | ||
| | grep -v "go.sum") | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid word-splitting with find; handle filenames safely.
Array via command substitution splits on whitespace. Use NUL-delimited mapfile.
Apply this diff:
- local files_to_check=(
- $(find . -type f \( -name "*.go" -o -name "*.yaml" -o -name "*.yml" -o -name "*.json" -o -name "*.md" -o -name "*.Dockerfile" \) \
- -not -path "./vendor/*" \
- -not -path "./.git/*" \
- -not -path "./testbin/*" \
- | grep -v "go.sum")
- )
+ local files_to_check=()
+ while IFS= read -r -d '' f; do
+ # Exclude go.sum via pattern here
+ [[ "$f" == *"/go.sum" ]] && continue
+ files_to_check+=("$f")
+ done < <(find . -type f \( -name "*.go" -o -name "*.yaml" -o -name "*.yml" -o -name "*.json" -o -name "*.md" -o -name "*.Dockerfile" \) \
+ -not -path "./vendor/*" -not -path "./.git/*" -not -path "./testbin/*" -print0)📝 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
| local files_to_check=( | |
| $(find . -type f \( -name "*.go" -o -name "*.yaml" -o -name "*.yml" -o -name "*.json" -o -name "*.md" -o -name "*.Dockerfile" \) \ | |
| -not -path "./vendor/*" \ | |
| -not -path "./.git/*" \ | |
| -not -path "./testbin/*" \ | |
| | grep -v "go.sum") | |
| ) | |
| local files_to_check=() | |
| while IFS= read -r -d '' f; do | |
| # Exclude go.sum via pattern here | |
| [[ "$f" == *"/go.sum" ]] && continue | |
| files_to_check+=("$f") | |
| done < <(find . -type f \( -name "*.go" -o -name "*.yaml" -o -name "*.yml" -o -name "*.json" -o -name "*.md" -o -name "*.Dockerfile" \) \ | |
| -not -path "./vendor/*" -not -path "./.git/*" -not -path "./testbin/*" -print0) |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 315-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 314-321, the current construction uses
command substitution to populate an array which will word-split filenames with
spaces; replace that with a NUL-delimited read into the array: run find with
-print0 (and keep the existing -not -path exclusions and replace the external
grep -v "go.sum" with a -not -name 'go.sum' or equivalent find predicate), then
pipe to mapfile -d '' -t files_to_check (or use while IFS= read -r -d '' and
append) so filenames containing whitespace or special chars are handled safely.
Comment on lines
+325
to
+372
| safe_replace_version() { | ||
| local file="$1" | ||
| local old_version="$2" | ||
| local new_version="$3" | ||
| local context="$4" | ||
|
|
||
| # Skip if file doesn't exist or isn't readable | ||
| [[ ! -f "$file" || ! -r "$file" ]] && return | ||
|
|
||
| # Create a temporary file for processing | ||
| local temp_file=$(mktemp) | ||
| cp "$file" "$temp_file" | ||
|
|
||
| # Specific patterns to replace (avoiding URLs and comments) | ||
| case "$context" in | ||
| "cert-manager") | ||
| # Replace cert-manager version in specific contexts (avoid URLs and comments) | ||
| sed -i "s/cert-manager v${old_version}/cert-manager v${new_version}/g" "$temp_file" | ||
| sed -i "s/cert-manager@v${old_version}/cert-manager@v${new_version}/g" "$temp_file" | ||
| # Update CSV description links - match any existing version and replace with new version | ||
| sed -i "s|\[cert-manager v[0-9]\+\.[0-9]\+\.[0-9]\+\](https://github.com/cert-manager/cert-manager/tree/v[0-9]\+\.[0-9]\+\.[0-9]\+)|\[cert-manager v${new_version}\](https://github.com/cert-manager/cert-manager/tree/v${new_version})|g" "$temp_file" | ||
| sed -i "s/cert-manager-acmesolver:v${old_version}/cert-manager-acmesolver:v${new_version}/g" "$temp_file" | ||
| sed -i "s/cert-manager-controller:v${old_version}/cert-manager-controller:v${new_version}/g" "$temp_file" | ||
| sed -i "s/cert-manager-webhook:v${old_version}/cert-manager-webhook:v${new_version}/g" "$temp_file" | ||
| sed -i "s/cert-manager-cainjector:v${old_version}/cert-manager-cainjector:v${new_version}/g" "$temp_file" | ||
| sed -i "s/cert-manager\/tree\/v${old_version}/cert-manager\/tree\/v${new_version}/g" "$temp_file" | ||
| sed -i "s/app\.kubernetes\.io\/version: v${old_version}/app.kubernetes.io\/version: v${new_version}/g" "$temp_file" | ||
| sed -i "s/OPERAND_IMAGE_VERSION[[:space:]]*=[[:space:]]*${old_version}/OPERAND_IMAGE_VERSION = ${new_version}/g" "$temp_file" | ||
| sed -i "s/value: ${old_version}$/value: ${new_version}/g" "$temp_file" | ||
| sed -i "s/RELEASE_BRANCH=v${old_version}/RELEASE_BRANCH=v${new_version}/g" "$temp_file" | ||
| # Avoid corrupting URLs in comments - only replace in specific image contexts | ||
| sed -i "s/\(quay\.io\/jetstack\/.*:\)v${old_version}/\1v${new_version}/g" "$temp_file" | ||
| ;; | ||
| "bundle") | ||
| # Replace bundle version in specific contexts (avoid URLs) | ||
| sed -i "s/\b${old_version}\b/${new_version}/g" "$temp_file" | ||
| ;; | ||
| esac | ||
|
|
||
| # Check if file was actually modified | ||
| if ! cmp -s "$file" "$temp_file"; then | ||
| mv "$temp_file" "$file" | ||
| return 0 | ||
| else | ||
| rm -f "$temp_file" | ||
| return 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Fix broken word-boundary regex and sed portability in safe_replace_version.
sed’s \b isn’t portable and won’t match as intended. Also escape pattern/replacement and use the wrapper.
Apply this diff:
- local temp_file=$(mktemp)
+ local temp_file
+ temp_file=$(mktemp)
@@
- sed -i "s/cert-manager v${old_version}/cert-manager v${new_version}/g" "$temp_file"
- sed -i "s/cert-manager@v${old_version}/cert-manager@v${new_version}/g" "$temp_file"
- # Update CSV description links - match any existing version and replace with new version
- sed -i "s|\[cert-manager v[0-9]\+\.[0-9]\+\.[0-9]\+\](https://github.com/cert-manager/cert-manager/tree/v[0-9]\+\.[0-9]\+\.[0-9]\+)|\[cert-manager v${new_version}\](https://github.com/cert-manager/cert-manager/tree/v${new_version})|g" "$temp_file"
- sed -i "s/cert-manager-acmesolver:v${old_version}/cert-manager-acmesolver:v${new_version}/g" "$temp_file"
- sed -i "s/cert-manager-controller:v${old_version}/cert-manager-controller:v${new_version}/g" "$temp_file"
- sed -i "s/cert-manager-webhook:v${old_version}/cert-manager-webhook:v${new_version}/g" "$temp_file"
- sed -i "s/cert-manager-cainjector:v${old_version}/cert-manager-cainjector:v${new_version}/g" "$temp_file"
- sed -i "s/cert-manager\/tree\/v${old_version}/cert-manager\/tree\/v${new_version}/g" "$temp_file"
- sed -i "s/app\.kubernetes\.io\/version: v${old_version}/app.kubernetes.io\/version: v${new_version}/g" "$temp_file"
- sed -i "s/OPERAND_IMAGE_VERSION[[:space:]]*=[[:space:]]*${old_version}/OPERAND_IMAGE_VERSION = ${new_version}/g" "$temp_file"
- sed -i "s/value: ${old_version}$/value: ${new_version}/g" "$temp_file"
- sed -i "s/RELEASE_BRANCH=v${old_version}/RELEASE_BRANCH=v${new_version}/g" "$temp_file"
- # Avoid corrupting URLs in comments - only replace in specific image contexts
- sed -i "s/\(quay\.io\/jetstack\/.*:\)v${old_version}/\1v${new_version}/g" "$temp_file"
+ local old_pat new_repl
+ old_pat=$(escape_sed_pat "$old_version")
+ new_repl=$(escape_sed_repl "$new_version")
+ sed_inplace "s|cert-manager v$old_pat|cert-manager v$new_repl|g" "$temp_file"
+ sed_inplace "s|cert-manager@v$old_pat|cert-manager@v$new_repl|g" "$temp_file"
+ # Update CSV description links - match any existing version and replace with new version
+ sed_inplace "s|\\[cert-manager v[0-9]+\\.[0-9]+\\.[0-9]+\\]\\(https://github.com/cert-manager/cert-manager/tree/v[0-9]+\\.[0-9]+\\.[0-9]+\\)|[cert-manager v$new_repl](https://github.com/cert-manager/cert-manager/tree/v$new_repl)|g" "$temp_file"
+ sed_inplace "s|cert-manager-acmesolver:v$old_pat|cert-manager-acmesolver:v$new_repl|g" "$temp_file"
+ sed_inplace "s|cert-manager-controller:v$old_pat|cert-manager-controller:v$new_repl|g" "$temp_file"
+ sed_inplace "s|cert-manager-webhook:v$old_pat|cert-manager-webhook:v$new_repl|g" "$temp_file"
+ sed_inplace "s|cert-manager-cainjector:v$old_pat|cert-manager-cainjector:v$new_repl|g" "$temp_file"
+ sed_inplace "s|cert-manager/tree/v$old_pat|cert-manager/tree/v$new_repl|g" "$temp_file"
+ sed_inplace "s|app\.kubernetes\.io/version:[[:space:]]*v$old_pat|app.kubernetes.io/version: v$new_repl|g" "$temp_file"
+ sed_inplace "s|(^|[[:space:]])OPERAND_IMAGE_VERSION[[:space:]]*=[[:space:]]*$old_pat|\1OPERAND_IMAGE_VERSION = $new_repl|g" "$temp_file"
+ sed_inplace "s|^([[:space:]]*value:[[:space:]])$old_pat$|\1$new_repl|g" "$temp_file"
+ sed_inplace "s|RELEASE_BRANCH=v$old_pat|RELEASE_BRANCH=v$new_repl|g" "$temp_file"
+ # Avoid corrupting URLs in comments - only replace in specific image contexts
+ sed_inplace "s|(quay\.io/jetstack/.+:)v$old_pat|\\1v$new_repl|g" "$temp_file"
;;
"bundle")
- # Replace bundle version in specific contexts (avoid URLs)
- sed -i "s/\b${old_version}\b/${new_version}/g" "$temp_file"
+ # Replace bundle version with conservative boundaries (non-word-ish separators)
+ local old_pat new_repl
+ old_pat=$(escape_sed_pat "$old_version")
+ new_repl=$(escape_sed_repl "$new_version")
+ sed_inplace "s|(^|[^0-9A-Za-z_.-])$old_pat([^0-9A-Za-z_.-]|$)|\\1$new_repl\\2|g" "$temp_file"
;;📝 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
| safe_replace_version() { | |
| local file="$1" | |
| local old_version="$2" | |
| local new_version="$3" | |
| local context="$4" | |
| # Skip if file doesn't exist or isn't readable | |
| [[ ! -f "$file" || ! -r "$file" ]] && return | |
| # Create a temporary file for processing | |
| local temp_file=$(mktemp) | |
| cp "$file" "$temp_file" | |
| # Specific patterns to replace (avoiding URLs and comments) | |
| case "$context" in | |
| "cert-manager") | |
| # Replace cert-manager version in specific contexts (avoid URLs and comments) | |
| sed -i "s/cert-manager v${old_version}/cert-manager v${new_version}/g" "$temp_file" | |
| sed -i "s/cert-manager@v${old_version}/cert-manager@v${new_version}/g" "$temp_file" | |
| # Update CSV description links - match any existing version and replace with new version | |
| sed -i "s|\[cert-manager v[0-9]\+\.[0-9]\+\.[0-9]\+\](https://github.com/cert-manager/cert-manager/tree/v[0-9]\+\.[0-9]\+\.[0-9]\+)|\[cert-manager v${new_version}\](https://github.com/cert-manager/cert-manager/tree/v${new_version})|g" "$temp_file" | |
| sed -i "s/cert-manager-acmesolver:v${old_version}/cert-manager-acmesolver:v${new_version}/g" "$temp_file" | |
| sed -i "s/cert-manager-controller:v${old_version}/cert-manager-controller:v${new_version}/g" "$temp_file" | |
| sed -i "s/cert-manager-webhook:v${old_version}/cert-manager-webhook:v${new_version}/g" "$temp_file" | |
| sed -i "s/cert-manager-cainjector:v${old_version}/cert-manager-cainjector:v${new_version}/g" "$temp_file" | |
| sed -i "s/cert-manager\/tree\/v${old_version}/cert-manager\/tree\/v${new_version}/g" "$temp_file" | |
| sed -i "s/app\.kubernetes\.io\/version: v${old_version}/app.kubernetes.io\/version: v${new_version}/g" "$temp_file" | |
| sed -i "s/OPERAND_IMAGE_VERSION[[:space:]]*=[[:space:]]*${old_version}/OPERAND_IMAGE_VERSION = ${new_version}/g" "$temp_file" | |
| sed -i "s/value: ${old_version}$/value: ${new_version}/g" "$temp_file" | |
| sed -i "s/RELEASE_BRANCH=v${old_version}/RELEASE_BRANCH=v${new_version}/g" "$temp_file" | |
| # Avoid corrupting URLs in comments - only replace in specific image contexts | |
| sed -i "s/\(quay\.io\/jetstack\/.*:\)v${old_version}/\1v${new_version}/g" "$temp_file" | |
| ;; | |
| "bundle") | |
| # Replace bundle version in specific contexts (avoid URLs) | |
| sed -i "s/\b${old_version}\b/${new_version}/g" "$temp_file" | |
| ;; | |
| esac | |
| # Check if file was actually modified | |
| if ! cmp -s "$file" "$temp_file"; then | |
| mv "$temp_file" "$file" | |
| return 0 | |
| else | |
| rm -f "$temp_file" | |
| return 1 | |
| fi | |
| } | |
| safe_replace_version() { | |
| local file="$1" | |
| local old_version="$2" | |
| local new_version="$3" | |
| local context="$4" | |
| # Skip if file doesn't exist or isn't readable | |
| [[ ! -f "$file" || ! -r "$file" ]] && return | |
| # Create a temporary file for processing | |
| local temp_file | |
| temp_file=$(mktemp) | |
| cp "$file" "$temp_file" | |
| case "$context" in | |
| "cert-manager") | |
| # Prepare escaped patterns for sed | |
| local old_pat new_repl | |
| old_pat=$(escape_sed_pat "$old_version") | |
| new_repl=$(escape_sed_repl "$new_version") | |
| sed_inplace "s|cert-manager v$old_pat|cert-manager v$new_repl|g" "$temp_file" | |
| sed_inplace "s|cert-manager@v$old_pat|cert-manager@v$new_repl|g" "$temp_file" | |
| # Update CSV description links - match any existing version and replace with new version | |
| sed_inplace "s|\\[cert-manager v[0-9]+\\.[0-9]+\\.[0-9]+\\]\\(https://github.com/cert-manager/cert-manager/tree/v[0-9]+\\.[0-9]+\\.[0-9]+\\)|[cert-manager v$new_repl](https://github.com/cert-manager/cert-manager/tree/v$new_repl)|g" "$temp_file" | |
| sed_inplace "s|cert-manager-acmesolver:v$old_pat|cert-manager-acmesolver:v$new_repl|g" "$temp_file" | |
| sed_inplace "s|cert-manager-controller:v$old_pat|cert-manager-controller:v$new_repl|g" "$temp_file" | |
| sed_inplace "s|cert-manager-webhook:v$old_pat|cert-manager-webhook:v$new_repl|g" "$temp_file" | |
| sed_inplace "s|cert-manager-cainjector:v$old_pat|cert-manager-cainjector:v$new_repl|g" "$temp_file" | |
| sed_inplace "s|cert-manager/tree/v$old_pat|cert-manager/tree/v$new_repl|g" "$temp_file" | |
| sed_inplace "s|app\.kubernetes\.io/version:[[:space:]]*v$old_pat|app.kubernetes.io/version: v$new_repl|g" "$temp_file" | |
| sed_inplace "s|(^|[[:space:]])OPERAND_IMAGE_VERSION[[:space:]]*=[[:space:]]*$old_pat|\1OPERAND_IMAGE_VERSION = $new_repl|g" "$temp_file" | |
| sed_inplace "s|^([[:space:]]*value:[[:space:]])$old_pat$|\1$new_repl|g" "$temp_file" | |
| sed_inplace "s|RELEASE_BRANCH=v$old_pat|RELEASE_BRANCH=v$new_repl|g" "$temp_file" | |
| # Avoid corrupting URLs in comments - only replace in specific image contexts | |
| sed_inplace "s|(quay\.io/jetstack/.+:)v$old_pat|\\1v$new_repl|g" "$temp_file" | |
| ;; | |
| "bundle") | |
| # Replace bundle version with conservative boundaries (non-word-ish separators) | |
| local old_pat new_repl | |
| old_pat=$(escape_sed_pat "$old_version") | |
| new_repl=$(escape_sed_repl "$new_version") | |
| sed_inplace "s|(^|[^0-9A-Za-z_.-])$old_pat([^0-9A-Za-z_.-]|$)|\\1$new_repl\\2|g" "$temp_file" | |
| ;; | |
| esac | |
| # Check if file was actually modified | |
| if ! cmp -s "$file" "$temp_file"; then | |
| mv "$temp_file" "$file" | |
| return 0 | |
| else | |
| rm -f "$temp_file" | |
| return 1 | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 335-335: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 325 to 372, the use of sed's \b is not
portable and also risks unescaped version strings being interpreted as regex
metacharacters; replace the fragile sed -i
"s/\b${old_version}\b/${new_version}/g" approach by (1) escaping the
old_version/new_version before embedding them into a regex (use a small helper
that uses printf '%s' to escape regex metachars), (2) use an explicit, portable
word-boundary pattern instead of \b (for example match
(^|[^A-Za-z0-9_.-])OLD([^A-Za-z0-9_.-]|$) and preserve surrounding chars in
backreferences), and (3) ensure sed -i portability by using the existing
temp-file copy/mv wrapper (already present) rather than relying on GNU-only sed
-i flags; update the "bundle" case to use that escaped-old_version and the
explicit-boundary sed expression so replacements only occur on whole-version
tokens.
Comment on lines
+455
to
+469
| # Parse command line arguments | ||
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| -h|--help) | ||
| usage | ||
| exit 0 | ||
| ;; | ||
| -d|--dry-run) | ||
| DRY_RUN=true | ||
| shift | ||
| ;; | ||
| -s|--step) | ||
| SPECIFIC_STEP="$2" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
Validate --step argument to avoid set -u crash.
With set -u, referencing $2 when missing will abort. Add a guard.
Apply this diff:
- -s|--step)
- SPECIFIC_STEP="$2"
- shift 2
+ -s|--step)
+ if [[ -n "${2-}" ]]; then
+ SPECIFIC_STEP="$2"
+ shift 2
+ else
+ log_error "Missing argument for --step. Expected 1-4."
+ exit 1
+ fi
;;📝 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
| # Parse command line arguments | |
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| -h|--help) | |
| usage | |
| exit 0 | |
| ;; | |
| -d|--dry-run) | |
| DRY_RUN=true | |
| shift | |
| ;; | |
| -s|--step) | |
| SPECIFIC_STEP="$2" | |
| shift 2 | |
| ;; | |
| # Parse command line arguments | |
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| -h|--help) | |
| usage | |
| exit 0 | |
| ;; | |
| -d|--dry-run) | |
| DRY_RUN=true | |
| shift | |
| ;; | |
| -s|--step) | |
| if [[ -n "${2-}" ]]; then | |
| SPECIFIC_STEP="$2" | |
| shift 2 | |
| else | |
| log_error "Missing argument for --step. Expected 1-4." | |
| exit 1 | |
| fi | |
| ;; |
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 455-469, the -s|--step case references $2
directly which under set -u will crash if the argument is missing; update the
case to first verify a next argument exists (e.g. check that $# -ge 2 or that
"${2-}" is non-empty and not another flag) and if missing print a usage/error
and exit with non-zero, otherwise assign SPECIFIC_STEP="$2" and shift 2; ensure
the guard handles values that look like flags so you don't treat them as the
step.
Contributor
|
@PillaiManish: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 tasks
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.
Summary by CodeRabbit