NO-JIRA: Script to automate rebase #309
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PillaiManish: 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. |
WalkthroughAdds a new Bash script Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to inspect closely:
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 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.
| # 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.
There was a problem hiding this comment.
Worth considering I think, including the subsequent commands, which uses the suggestion.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all sed command usages in the rebase_automation.sh file
echo "=== All sed command usages in rebase_automation.sh ==="
rg -n 'sed\s' rebase_automation.sh
echo -e "\n=== Count of sed -i usages ==="
rg -c 'sed\s+-i' rebase_automation.sh || echo "0 occurrences"
echo -e "\n=== Checking for BRE patterns (backslash-escaped operators) ==="
rg -n '\\+|\\{|\\}|\\\(' rebase_automation.sh | head -20
Length of output: 3175
🏁 Script executed:
#!/bin/bash
# Look for BRE patterns more carefully
echo "=== Checking for backslash-escaped regex operators (BRE patterns) ==="
rg -n '\\\\+' rebase_automation.sh
echo -e "\n=== Checking for \\1 backreferences ==="
rg -n '\\\\1|\\\\2|\\\\3' rebase_automation.sh
echo -e "\n=== All sed -i lines (showing full context) ==="
rg -n -A1 'sed -i' rebase_automation.sh
Length of output: 3473
@bharath-b-rh Great! Here's a comprehensive guide for applying the sed portability helpers throughout the script:
Implementation Steps
1. Add the helper functions after log_error() (around line 39):
# --- 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'; }2. Replace all 23 sed -i calls with sed_inplace:
- Lines 202, 206, 210 (Makefile updates)
- Lines 254, 257, 260, 263 (CSV updates)
- Line 274 (bundle.Dockerfile)
- Line 282 (bundle/metadata/annotations.yaml)
- Lines 342-356, 360 (Step 4 manual replacements)
3. Convert BRE patterns to ERE (Extended Regular Expressions):
- Line 260: Change
[0-9]\+→[0-9]+(one or more digits) - Line 345: Change
[0-9]\+→[0-9]+(appears 6 times in that line) - Line 356: Change
\(quay\.io\/jetstack\/.*:\)→(quay\.io/jetstack/.*:)(capture group) - Line 360: Change
\b→[[:<:]]and\b→[[:>:]]for word boundaries (or use\<and\>)
4. Consider escaping version variables where they're used in patterns (optional but recommended for robustness):
# Example for line 202:
old_escaped=$(escape_sed_pat "$OLD_BUNDLE_VERSION")
new_escaped=$(escape_sed_repl "$NEW_BUNDLE_VERSION")
sed_inplace "s/^BUNDLE_VERSION ?= $old_escaped/BUNDLE_VERSION ?= $new_escaped/" MakefileThis ensures the script works correctly on both macOS/BSD and Linux/GNU systems, and handles edge cases where version numbers might contain regex special characters.
| 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.
| # 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.
| # 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.
| 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.
| 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.
| 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.
| 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.
| 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.
| # 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.
| # 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.
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
LGTM
Could we extend this script for other operands like istio-csr (in future trust-manager, approver-policy) as well.
rebase_automation.sh
Outdated
|
|
||
| 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) |
There was a problem hiding this comment.
nit: examples have different versions in usage.
PillaiManish
force-pushed
the
automate-rebase-chores
branch
from
d6a3d3a to
7ba3b14
Compare
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
rebase_automation.sh (1)
95-106: Add version format validation to prevent invalid input.The script checks if
NEW_CERT_MANAGER_VERSIONandNEW_BUNDLE_VERSIONare set but doesn't validate their format. Invalid values (e.g., "abc", "1.x.y", or strings with special regex characters) will pass this check and later break sed patterns, corrupt files, or causego getfailures.🔎 Proposed validation logic
if [[ -z "$NEW_CERT_MANAGER_VERSION" ]]; then log_error "NEW_CERT_MANAGER_VERSION is not set" log_info "Example: export NEW_CERT_MANAGER_VERSION=1.19.0" exit 1 fi + + # Validate version format (semantic versioning: X.Y.Z) + if [[ ! "$NEW_CERT_MANAGER_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + log_error "NEW_CERT_MANAGER_VERSION has invalid format: $NEW_CERT_MANAGER_VERSION" + log_info "Expected format: X.Y.Z (e.g., 1.19.0)" + exit 1 + fi if [[ -z "$NEW_BUNDLE_VERSION" ]]; then log_error "NEW_BUNDLE_VERSION is not set" log_info "Example: export NEW_BUNDLE_VERSION=1.19.0" exit 1 fi + + # Validate version format (semantic versioning: X.Y.Z) + if [[ ! "$NEW_BUNDLE_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + log_error "NEW_BUNDLE_VERSION has invalid format: $NEW_BUNDLE_VERSION" + log_info "Expected format: X.Y.Z (e.g., 1.19.0)" + exit 1 + fi
📜 Review details
Configuration used: Organization 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
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
rebase_automation.sh
🪛 Shellcheck (0.11.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)
| if [[ "$SKIP_COMMIT" != "true" ]]; then | ||
| git add . | ||
| git commit -m "Update Makefile: BUNDLE_VERSION, CERT_MANAGER_VERSION, CHANNELS" | ||
| log_success "Step 2 committed" |
There was a problem hiding this comment.
Replace git add . with explicit file patterns.
Using git add . stages all changes in the repository, including untracked files, which could accidentally commit build artifacts, editor temporary files, secrets, or unrelated work. This is a security and data integrity risk.
🔎 Recommended fix for Step 2
# Commit changes
if [[ "$SKIP_COMMIT" != "true" ]]; then
- git add .
+ git add Makefile config/ bundle/ bindata/
git commit -m "Update Makefile: BUNDLE_VERSION, CERT_MANAGER_VERSION, CHANNELS"
log_success "Step 2 committed"
fiNote: Adjust the explicit paths based on what make update and make bundle actually modify. Use git status --porcelain to determine the expected files.
📝 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.
| if [[ "$SKIP_COMMIT" != "true" ]]; then | |
| git add . | |
| git commit -m "Update Makefile: BUNDLE_VERSION, CERT_MANAGER_VERSION, CHANNELS" | |
| log_success "Step 2 committed" | |
| if [[ "$SKIP_COMMIT" != "true" ]]; then | |
| git add Makefile config/ bundle/ bindata/ | |
| git commit -m "Update Makefile: BUNDLE_VERSION, CERT_MANAGER_VERSION, CHANNELS" | |
| log_success "Step 2 committed" |
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 220-223, replace the unsafe broad "git add
." with explicit file patterns so only the files modified by the update (e.g.,
Makefile and the bundle/manifests that make update/make bundle touch) are
staged; run git status --porcelain or inspect the make targets to determine the
exact paths, then use git add <explicit-paths> (comma/separated or multiple
args) for those files (for example: Makefile and the specific bundle/ or charts/
directories) before committing, ensuring untracked/editor/temp files are not
included.
| # 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 |
There was a problem hiding this comment.
Replace git add . with explicit file patterns.
Using git add . stages all changes in the repository, including untracked files, which could accidentally commit build artifacts, editor temporary files, secrets, or unrelated work. This is a security and data integrity risk.
🔎 Recommended fix for Step 3
# Commit changes
if [[ "$SKIP_COMMIT" != "true" ]]; then
- git add .
+ git add config/ bundle/ bindata/
git commit -m "Update CSV: OLM bundle name, version, replaces, skipRange and skips"
log_success "Step 3 committed"
fiNote: Adjust the explicit paths based on what make update-bindata modifies. Use git status --porcelain to determine the expected files.
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 289 to 294, replace the unsafe git add .
(which stages all changes including untracked files) with explicit git add of
only the files/paths that Step 3 legitimately modifies; determine those paths
via git status --porcelain (likely the generated CSV/OLM bundle files and
bindata outputs) and add those specific patterns/paths (or use git add --
<file1> <file2> ...) so only the intended files are committed, then run git
commit as before.
| # Commit all changes (manual replacements + generated files) | ||
| if [[ "$SKIP_COMMIT" != "true" ]] && [[ -n "$(git status --porcelain)" ]]; then | ||
| git add . | ||
| git commit -m "More manual replacements" | ||
| log_success "Step 4 committed" | ||
| else |
There was a problem hiding this comment.
Replace git add . with explicit file patterns.
Using git add . stages all changes in the repository, including untracked files, which could accidentally commit build artifacts, editor temporary files, secrets, or unrelated work. This is a security and data integrity risk.
🔎 Recommended fix for Step 4
# Commit all changes (manual replacements + generated files)
if [[ "$SKIP_COMMIT" != "true" ]] && [[ -n "$(git status --porcelain)" ]]; then
- git add .
+ # Add only the files that were actually modified by replacements
+ if [[ ${#changed_files[@]} -gt 0 ]]; then
+ git add "${changed_files[@]}"
+ fi
+ # Add generated files from make manifests bundle
+ git add config/ bundle/ bindata/ 2>/dev/null || true
git commit -m "More manual replacements"
log_success "Step 4 committed"
elseAlternatively, use git status --porcelain to identify modified tracked files and avoid staging untracked files:
if [[ "$SKIP_COMMIT" != "true" ]] && [[ -n "$(git status --porcelain)" ]]; then
- git add .
+ git add -u # Only stage modified tracked files, not new untracked files
git commit -m "More manual replacements"
log_success "Step 4 committed"📝 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.
| # Commit all changes (manual replacements + generated files) | |
| if [[ "$SKIP_COMMIT" != "true" ]] && [[ -n "$(git status --porcelain)" ]]; then | |
| git add . | |
| git commit -m "More manual replacements" | |
| log_success "Step 4 committed" | |
| else | |
| # Commit all changes (manual replacements + generated files) | |
| if [[ "$SKIP_COMMIT" != "true" ]] && [[ -n "$(git status --porcelain)" ]]; then | |
| git add -u # Only stage modified tracked files, not new untracked files | |
| git commit -m "More manual replacements" | |
| log_success "Step 4 committed" | |
| else |
🤖 Prompt for AI Agents
In rebase_automation.sh around lines 420-425, the script currently uses "git add
." which stages all changes including untracked files; replace this with a safe
approach that only stages intended tracked modifications (e.g., use git add -u /
git add --update or parse git status --porcelain to build an explicit list of
modified/removed tracked files) or explicitly list the file patterns produced by
the script; ensure untracked files are not staged, then commit with the same
message and keep the existing SKIP_COMMIT check and success log.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
rebase_automation.sh (5)
124-133: Version auto-detection may be brittle with spacing/quote variations.The
grep | cut | trpipeline can fail on Makefile variations. A previous review comment suggested usingawkfor more robust parsing.
220-224: Critical: Replacegit add .with explicit file patterns.This stages all changes including untracked files, potentially committing secrets, build artifacts, or editor temp files. A previous review comment already flagged this issue.
289-294: Critical: Replacegit add .with explicit file patterns.Same security concern as Step 2. A previous review comment already flagged this issue.
420-425: Critical: Replacegit add .with explicit file patterns orgit add -u.This stages all changes including untracked files. A previous review comment already flagged this issue and suggested using
git add -uto only stage modified tracked files.
466-469: Validate--stepargument before accessing$2.With
set -u, accessing$2when missing will abort. A previous review comment already flagged this and suggested adding a guard.
🧹 Nitpick comments (2)
rebase_automation.sh (2)
197-198: Split declaration and assignment to avoid masking return values.Per SC2155, declare locals separately from assignment to catch failures.
🔎 Proposed fix
- 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)
407-408: Usemapfileto avoid word-splitting on filenames with spaces.Per SC2207, command substitution into an array will split on whitespace.
🔎 Proposed fix
- local unique_files=($(printf '%s\n' "${changed_files[@]}" | sort -u)) + local unique_files=() + mapfile -t unique_files < <(printf '%s\n' "${changed_files[@]}" | sort -u)
📜 Review details
Configuration used: Organization 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
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
rebase_automation.sh
🪛 Shellcheck (0.11.0)
rebase_automation.sh
[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 (5)
rebase_automation.sh (5)
1-33: LGTM!Good use of strict mode (
set -euo pipefail) and well-structured logging helpers with colored output.
35-70: LGTM!Clear and comprehensive usage documentation with examples and step descriptions.
72-117: LGTM!Solid prerequisite validation including git repo check, tool availability, environment variables, and semver format validation.
173-179: Good practice: explicit file patterns ingit add.Using
git add go.mod go.sum vendor/instead ofgit add .is the correct approach, avoiding accidental staging of unrelated files.
432-447: LGTM!Clean orchestration function with clear summary output.
| for file in "${files_to_check[@]}"; do | ||
| # Skip files that contain URLs or comments that might be corrupted | ||
| if grep -q "$OLD_CERT_MANAGER_VERSION" "$file" 2>/dev/null; then | ||
| # Skip if this looks like a URL corruption case | ||
| if grep -q "https://.*${OLD_CERT_MANAGER_VERSION}" "$file" 2>/dev/null; then | ||
| log_info "Skipping $file - contains URLs that might be corrupted" | ||
| continue | ||
| fi | ||
|
|
||
| if safe_replace_version "$file" "$OLD_CERT_MANAGER_VERSION" "$NEW_CERT_MANAGER_VERSION" "cert-manager"; then | ||
| log_info "Updated cert-manager version in $file" | ||
| changed_files+=("$file") | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
URL detection may skip legitimate files needing updates.
The logic at lines 380-383 skips any file containing https://.*${OLD_VERSION}, but this could skip files that have both URLs (which should remain unchanged) and non-URL version references (which should be updated). Consider a more targeted approach that replaces non-URL contexts while preserving URLs.
|
@PillaiManish: 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. |
4 participants
Summary by CodeRabbit