.claude: add pre-branching plugin #4856
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughAdds a new Claude plugin manifest and documentation for OpenShift pre-branching automation, plus nine executable Bash skills that implement branch creation, config copying/updating, gating/job updates, validation, Prow regeneration, analysis-job bumps, and commit creation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
🔇 Additional comments (4)
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: 8
🧹 Nitpick comments (7)
.claude/.claude-plugin/skills/download-sippy-config.sh (1)
1-31: Consider adding retry logic for network resilience.The script uses curl without retry mechanism. Transient network failures during download could cause unexpected script termination. Since this is run as part of a multi-step workflow, consider adding exponential backoff retry logic to improve resilience.
# Consider adding a retry wrapper: retry_curl() { local max_attempts=3 local timeout=10 local attempt=1 local delay=1 while [ $attempt -le $max_attempts ]; do if curl -sSL --max-time "$timeout" -o "$1" "$2"; then return 0 fi if [ $attempt -lt $max_attempts ]; then echo "Download failed, retrying in ${delay}s..." sleep "$delay" delay=$((delay * 2)) fi attempt=$((attempt + 1)) done return 1 } # Then use: retry_curl "${OUTPUT_PATH}" "https://raw.githubusercontent.com/...".claude/.claude-plugin/skills/validate-release-controller-config.sh (1)
36-36: Clarify the error message context.Line 36 mentions "manual job copying may be required" as a warning when validation fails, but this message is ambiguous—it's unclear when manual intervention is needed versus when it should fail the entire operation. Either clarify the condition or consider whether this should fail faster to prevent partial execution.
Consider either:
- Remove the soft-failure messaging and let the validation script's exit code determine the flow.
- Document in comments when manual intervention is needed.
.claude/.claude-plugin/skills/create-pre-branching-branch.sh (1)
23-28: Add validation that release repository exists and is a Git repository.The script assumes
RELEASE_REPOis a valid Git repository but doesn't verify this before attempting operations. Early validation improves error messages for users.cd "${RELEASE_REPO}" # Add early validation after cd: +if ! git rev-parse --git-dir > /dev/null 2>&1; then + echo "✗ Error: '${RELEASE_REPO}' is not a valid Git repository" + exit 1 +fi.claude/commands/pre-branching.md (1)
158-164: Clarify template variable substitution mechanism.The documentation uses template variables (e.g.,
{{OLD_VERSION}}) but doesn't explain how/when they get substituted. Are these:
- Substituted by Claude before presenting to user?
- Substituted by the plugin manifest system?
- Expected to be manually replaced by the user?
This is crucial for users following this workflow. Add a sentence clarifying the substitution mechanism, ideally in a prominent "Setup" or "Before You Start" section.
Add a setup section at the beginning:
## Setup Before running these commands, ensure the following template variables are set: - `{{OLD_VERSION}}`: Previous release version (e.g., "4.21") - `{{NEW_VERSION}}`: New release version (e.g., "4.22") - `{{RELEASE_REPO}}`: Absolute path to your release repository clone - `{{CI_TOOLS_REPO}}`: Absolute path to your ci-tools repository clone [Explain how these get substituted: by Claude, by shell, manually by user, etc.].claude/.claude-plugin/skills/create-pre-branching-commits.sh (2)
41-44: Hardcoded Sippy config path in commit message may become stale.Line 44 hardcodes the Sippy config path
/tmp/sippy-openshift.yamlin the commit message. This path was passed as an argument to the skill but is then hardcoded in the commit message. If users change the download location (e.g., via the optional parameter indownload-sippy-config.sh), the commit message will be inaccurate.Either:
- Accept
SIPPY_CONFIGas a 4th argument to this script, or- Use a command-substitution to extract the actual Sippy config path from recent git changes, or
- Document in comments that the hardcoded path assumes default behavior
For now, consider adding a 4th optional parameter:
+SIPPY_CONFIG="${4:-/tmp/sippy-openshift.yaml}" # Then in line 44: -Generated by: -./generated-release-gating-jobs --current-release=${OLD_VERSION} --release-repo=${RELEASE_REPO} --sippy-config=/tmp/sippy-openshift.yaml --interval=168" +Generated by: +./generated-release-gating-jobs --current-release=${OLD_VERSION} --release-repo=${RELEASE_REPO} --sippy-config=${SIPPY_CONFIG} --interval=168"
26-29: Simplify complex git change detection condition.Line 26 combines three git conditions with
&&which is somewhat hard to parse. While functionally correct, consider extracting into a helper function for clarity, or usegit status --porcelainwhich may be more readable:-if git diff --quiet && git diff --cached --quiet && [ -z "$(git ls-files --others --exclude-standard)" ]; then +# Check if there are any uncommitted changes (staged, unstaged, or untracked) +if git diff-index --quiet HEAD -- && [ -z "$(git ls-files --others --exclude-standard)" ]; then echo "No changes to commit!" exit 1 fiOr alternatively:
+HAS_CHANGES=$(git status --porcelain | grep -c '^' || true) +if [ "$HAS_CHANGES" -eq 0 ]; then + echo "No changes to commit!" + exit 1 +fi.claude/.claude-plugin/skills/bump-analysis-jobs.sh (1)
50-51: Document YAML parsing assumptions and add robustness checks.The regex pattern (line 50) assumes strict YAML formatting with 2-space indentation and a specific structure (
\n- .*?\n name: {job_name}\n). If YAML formatting varies, job extraction may silently fail or extract incomplete blocks.Add comments explaining YAML structure assumptions, and consider adding validation of extracted YAML blocks:
def extract_jobs(file_path, job_names): """Extract complete job definitions for given job names from a YAML file""" + # NOTE: This assumes YAML list items with 2-space indentation + # and 'name:' field at depth 1. Breaks if indentation differs. with open(file_path, 'r') as f: content = f.read() jobs = [] for job_name in job_names: - # Find the job by name - match from "- " to the next "- " or end of file + # Find the job by name - match from "- " to the next "- " or end of file. + # Pattern assumes 2-space indentation (YAML standard but not validated). pattern = rf'(\n- .*?\n name: {re.escape(job_name)}\n.*?)(?=\n- |\Z)' match = re.search(pattern, content, re.DOTALL) if match: jobs.append(match.group(1)) print(f" ✓ Found job: {job_name}", file=sys.stderr) + # Optionally validate extracted block is valid YAML list item else: print(f" ✗ Could not find job: {job_name}", file=sys.stderr)Alternatively, consider using a proper YAML parser (e.g.,
PyYAML) for more robust parsing.
📜 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 (10)
.claude/.claude-plugin/plugin.json(1 hunks).claude/.claude-plugin/skills/bump-analysis-jobs.sh(1 hunks).claude/.claude-plugin/skills/copy-release-controller-configs.sh(1 hunks).claude/.claude-plugin/skills/create-pre-branching-branch.sh(1 hunks).claude/.claude-plugin/skills/create-pre-branching-commits.sh(1 hunks).claude/.claude-plugin/skills/download-sippy-config.sh(1 hunks).claude/.claude-plugin/skills/regenerate-prow-jobs.sh(1 hunks).claude/.claude-plugin/skills/update-release-gating-jobs.sh(1 hunks).claude/.claude-plugin/skills/validate-release-controller-config.sh(1 hunks).claude/commands/pre-branching.md(1 hunks)
🔇 Additional comments (2)
.claude/.claude-plugin/skills/regenerate-prow-jobs.sh (1)
1-30: Verify prerequisites (make, go) are available.The script assumes
makeand underlying build tools are available but doesn't verify they exist. When invoked in unfamiliar environments, the error messages may be cryptic if prerequisites are missing.Consider adding a prerequisite check:
# After set -euo pipefail, add: if ! command -v make &> /dev/null; then echo "✗ Error: 'make' not found in PATH" exit 1 fiVerify that the prerequisite chain for
make jobs(e.g., Go, Python, etc.) is well-documented or checked..claude/.claude-plugin/plugin.json (1)
11-11: Verify all skill scripts and settings file are included in the PR.The manifest references a settings file
settings.pre-branching.json(line 11) and eight skill scripts (e.g.,skills/create-pre-branching-branch.shat line 22). Confirm these files are present in the PR.Also applies to: 18-103
| "RELEASE_REPO": "/home/prucek/work/release", | ||
| "CI_TOOLS_REPO": "/home/prucek/work/ci-tools" |
There was a problem hiding this comment.
Replace hardcoded developer paths with environment variables or relative paths.
Lines 105-106 contain absolute paths specific to the developer's machine (/home/prucek/work/...). These will not work for other developers or CI environments and appear to be accidentally committed developer config.
Use environment variables or relative paths instead. For example:
- "config": {
- "RELEASE_REPO": "/home/prucek/work/release",
- "CI_TOOLS_REPO": "/home/prucek/work/ci-tools"
- }
+ "config": {
+ "RELEASE_REPO": "${RELEASE_REPO:-../release}",
+ "CI_TOOLS_REPO": "${CI_TOOLS_REPO:-../ci-tools}"
+ }Alternatively, document in README how users should configure these paths before running the plugin, or derive them from the current working directory.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .claude/.claude-plugin/plugin.json around lines 105-106 there are hardcoded
absolute developer paths (/home/prucek/work/release and
/home/prucek/work/ci-tools); replace these with configurable values by reading
environment variables (e.g., RELEASE_REPO and CI_TOOLS_REPO) or using paths
relative to the repository root (e.g., ./release, ./ci-tools) and provide
sensible fallbacks if unset; update the plugin to load those env vars or
construct relative paths at runtime and add instructions to the README (or a
README section) describing how to set the environment variables or where to
place the directories so CI and other developers don’t rely on machine-specific
paths.
| def bump_analysis_job(job_text, old_ver, new_ver): | ||
| """Replace version numbers in a job definition""" | ||
| return job_text.replace(old_ver, new_ver) |
There was a problem hiding this comment.
Replace naive string replacement with semantic version bumping.
The bump_analysis_job function uses string.replace() to swap old and new versions (line 40), which will replace all occurrences of the old_version throughout the entire job block. This risks corrupting job definitions if the version string appears in unintended contexts (e.g., image URIs, configuration values, comments).
Use targeted regex replacement that only updates version strings in specific YAML fields. For example:
def bump_analysis_job(job_text, old_ver, new_ver):
- """Replace version numbers in a job definition"""
- return job_text.replace(old_ver, new_ver)
+ """Replace version numbers in specific YAML fields"""
+ # Only replace version in predictable fields (name, image tag refs, etc.)
+ # Avoid replacing in unrelated config or comments
+ result = job_text
+ # Replace in job name field
+ result = re.sub(
+ rf'(\n name:.*?{re.escape(old_ver)})',
+ lambda m: m.group(1).replace(old_ver, new_ver),
+ result
+ )
+ # Add similar targeted replacements for other fields as needed
+ return resultCommittable suggestion skipped: line range outside the PR's diff.
| def append_bumped_jobs(file_path, job_names, old_ver, new_ver): | ||
| """Extract jobs, bump versions, and append to the file""" | ||
| print(f"\nProcessing {file_path}...", file=sys.stderr) | ||
| jobs = extract_jobs(file_path, job_names) | ||
|
|
||
| if not jobs: | ||
| print(f" No jobs found in {file_path}", file=sys.stderr) | ||
| return False | ||
|
|
||
| # Read the current file | ||
| with open(file_path, 'r') as f: | ||
| content = f.read() | ||
|
|
||
| # Bump versions in the extracted jobs | ||
| bumped_jobs = [bump_analysis_job(job, old_ver, new_ver) for job in jobs] | ||
|
|
||
| # Append the bumped jobs to the file | ||
| new_content = content.rstrip() + '\n' + '\n'.join(bumped_jobs) + '\n' | ||
|
|
||
| with open(file_path, 'w') as f: | ||
| f.write(new_content) | ||
|
|
||
| print(f" ✓ Added {len(bumped_jobs)} bumped jobs", file=sys.stderr) | ||
| return True |
There was a problem hiding this comment.
Add idempotency guards to prevent duplicate job entries on repeated runs.
The append_bumped_jobs function unconditionally appends bumped jobs to the YAML file (line 77) without checking if they already exist. Running this script multiple times will create duplicate job entries, corrupting the configuration.
Modify the function to check if the bumped jobs already exist in the file before appending. For example:
def append_bumped_jobs(file_path, job_names, old_ver, new_ver):
"""Extract jobs, bump versions, and append to the file"""
print(f"\nProcessing {file_path}...", file=sys.stderr)
jobs = extract_jobs(file_path, job_names)
if not jobs:
print(f" No jobs found in {file_path}", file=sys.stderr)
return False
# Read the current file
with open(file_path, 'r') as f:
content = f.read()
+ # Bump versions in the extracted jobs
+ bumped_jobs = [bump_analysis_job(job, old_ver, new_ver) for job in jobs]
+
+ # Check if bumped jobs already exist in file to avoid duplicates
+ existing_jobs = '\n'.join(bumped_jobs) in content
+ if existing_jobs:
+ print(f" ℹ Bumped jobs already exist in {file_path}, skipping append", file=sys.stderr)
+ return True
+
# Bump versions in the extracted jobs
- bumped_jobs = [bump_analysis_job(job, old_ver, new_ver) for job in jobs]
# Append the bumped jobs to the file
new_content = content.rstrip() + '\n' + '\n'.join(bumped_jobs) + '\n'Alternatively, replace entire job entries instead of appending, or use a more robust YAML library that maintains referential integrity.
🤖 Prompt for AI Agents
In .claude/.claude-plugin/skills/bump-analysis-jobs.sh around lines 60–83, the
function append_bumped_jobs currently unconditionally appends bumped job YAML
fragments which causes duplicate job entries on repeated runs; update it to be
idempotent by detecting existing bumped jobs and either skipping or replacing
them before writing. Specifically, parse the current file (prefer using a YAML
loader) to identify jobs by their unique key/name/ID, for each bumped job check
if an entry with the same key already exists and if so replace that entry (or
skip appending if identical), otherwise append the new job; only write the
merged/updated YAML back to disk so repeated runs do not create duplicates.
| multiarch_file = 'ci-operator/jobs/openshift/multiarch/openshift-multiarch-master-periodics.yaml' | ||
| multiarch_jobs = [ | ||
| f'periodic-ci-openshift-multiarch-master-nightly-{old_version}-install-analysis-all-multi-p-p', | ||
| f'periodic-ci-openshift-multiarch-master-nightly-{old_version}-install-analysis-all-ppc64le', | ||
| f'periodic-ci-openshift-multiarch-master-nightly-{old_version}-install-analysis-all-s390x', | ||
| ] | ||
|
|
||
| print("\n=== Multiarch Analysis Jobs ===", file=sys.stderr) | ||
| append_bumped_jobs(multiarch_file, multiarch_jobs, old_version, new_version) | ||
|
|
||
| # Bump release jobs | ||
| release_file = 'ci-operator/jobs/openshift/release/openshift-release-master-periodics.yaml' |
There was a problem hiding this comment.
Add file existence checks before read/write operations.
The script assumes both YAML files exist at lines 86 and 97 but does not validate their presence. If a file is missing, the script will fail with a cryptic Python traceback instead of a clear error message.
Add existence checks before processing:
+ # Verify files exist
+ multiarch_file = 'ci-operator/jobs/openshift/multiarch/openshift-multiarch-master-periodics.yaml'
+ release_file = 'ci-operator/jobs/openshift/release/openshift-release-master-periodics.yaml'
+
+ if [ ! -f "$multiarch_file" ]; then
+ echo "Error: Multiarch jobs file not found: $multiarch_file" >&2
+ exit 1
+ fi
+ if [ ! -f "$release_file" ]; then
+ echo "Error: Release jobs file not found: $release_file" >&2
+ exit 1
+ fiOr add validation in Python before opening files.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .claude/.claude-plugin/skills/bump-analysis-jobs.sh around lines 86 to 97 the
script calls append_bumped_jobs on YAML paths without checking the files exist;
add explicit existence checks for multiarch_file and release_file before any
read/write operations and fail fast with a clear error message if either is
missing. Implement this by testing for file presence (e.g., os.path.exists or
shell test -f depending on runtime) prior to calling append_bumped_jobs, and
return or exit with a descriptive error that includes the missing path so
subsequent file reads/writes won’t produce a cryptic traceback.
| ] | ||
|
|
||
| print("\n=== Multiarch Analysis Jobs ===", file=sys.stderr) | ||
| append_bumped_jobs(multiarch_file, multiarch_jobs, old_version, new_version) |
There was a problem hiding this comment.
Check return values from append_bumped_jobs to detect failures.
Lines 94 and 105 call append_bumped_jobs but ignore its return value. If jobs are not found in a file, the function returns False, but the script continues and still exits with status 0, masking the failure.
Capture and handle the return value:
print("\n=== Multiarch Analysis Jobs ===", file=sys.stderr)
-append_bumped_jobs(multiarch_file, multiarch_jobs, old_version, new_version)
+if not append_bumped_jobs(multiarch_file, multiarch_jobs, old_version, new_version):
+ print("Warning: No multiarch jobs were processed", file=sys.stderr)
# Bump release jobs
release_file = 'ci-operator/jobs/openshift/release/openshift-release-master-periodics.yaml'
@@ -102,7 +102,9 @@
]
print("\n=== Release Analysis Jobs ===", file=sys.stderr)
-append_bumped_jobs(release_file, release_jobs, old_version, new_version)
+if not append_bumped_jobs(release_file, release_jobs, old_version, new_version):
+ print("Error: No release jobs were processed", file=sys.stderr)
+ sys.exit(1)Consider whether missing jobs should be warnings (non-fatal) or errors (fatal) based on your workflow expectations.
Also applies to: 105-105
🤖 Prompt for AI Agents
.claude/.claude-plugin/skills/bump-analysis-jobs.sh around lines 94 and 105: the
script calls append_bumped_jobs(...) but ignores its boolean return value, so
failures (returns False when jobs not found) are masked; update both call sites
to capture the return value, check it, and handle failures according to policy —
either emit a warning and continue or log an error and exit with a non-zero
status (prefer exiting if missing jobs should be fatal); ensure the log message
includes which file and versions failed and return/exit appropriately so the
script surface failures instead of exiting 0.
| OLD_VERSION="$1" | ||
| NEW_VERSION="$2" | ||
| RELEASE_REPO="$3" | ||
| RELEASES_DIR="${RELEASE_REPO}/core-services/release-controller/_releases" | ||
|
|
||
| echo "Bumping release controller configs: ${OLD_VERSION} → ${NEW_VERSION}" | ||
|
|
||
| # Escape dots for sed patterns | ||
| OLD_ESCAPED="${OLD_VERSION//./\\.}" | ||
|
|
||
| # Calculate previous version for nested references | ||
| PREV_MINOR=$((${OLD_VERSION##*.} - 1)) | ||
| PREV_VERSION="${OLD_VERSION%.*}.${PREV_MINOR}" | ||
| PREV_ESCAPED="${PREV_VERSION//./\\.}" | ||
| CURR_MINOR=$((${NEW_VERSION##*.} - 1)) | ||
| CURR_VERSION="${NEW_VERSION%.*}.${CURR_MINOR}" | ||
|
|
||
| # Find all files with old_version in their name and bump them | ||
| find "$RELEASES_DIR" -type f -name "*${OLD_VERSION}*.json" 2>/dev/null | while read -r src_file; do | ||
| # Skip if already processed/destination exists | ||
| dst_file="${src_file//${OLD_VERSION}/${NEW_VERSION}}" | ||
| [ -f "$dst_file" ] && continue | ||
|
|
||
| echo " $(basename "$src_file") → $(basename "$dst_file")" | ||
| cp "$src_file" "$dst_file" | ||
|
|
||
| # Bump version strings inside the file | ||
| sed -i "s/${OLD_ESCAPED}/${NEW_VERSION}/g" "$dst_file" | ||
| sed -i "s/${PREV_ESCAPED}/${CURR_VERSION}/g" "$dst_file" |
There was a problem hiding this comment.
Address fragile version parsing and sed backup risk.
Multiple issues in this section:
-
Version parsing assumes X.Y format (Lines 25-28): The arithmetic
PREV_MINOR=$((${OLD_VERSION##*.} - 1))will fail or produce unexpected results for versions like"4"(no dot),"4.21.1"(too many dots), or when subtracting results in negative numbers. For example, version"4.0"would producePREV_MINOR = -1. -
sed -i without backup (Line 41-42): In-place sed edits without backup (with
-i .bakor similar) risk data corruption if sed fails. The script usesset -euo pipefailbut sed errors within the file loop won't necessarily stop execution. -
Filename pattern matching could be too broad (Line 32): Files matching
*${OLD_VERSION}*.jsoncould unintentionally match unrelated files. For example, version"4.2"would match"4.21"files.
Consider this improved version:
-# Calculate previous version for nested references
-PREV_MINOR=$((${OLD_VERSION##*.} - 1))
-PREV_VERSION="${OLD_VERSION%.*}.${PREV_MINOR}"
+# Validate version format and calculate previous version
+if [[ ! "$OLD_VERSION" =~ ^[0-9]+\.[0-9]+$ ]]; then
+ echo "✗ Error: OLD_VERSION must be in X.Y format (e.g., 4.21)"
+ exit 1
+fi
+
+MAJOR="${OLD_VERSION%.*}"
+MINOR="${OLD_VERSION##*.}"
+PREV_MINOR=$((MINOR - 1))
+if [ "$PREV_MINOR" -lt 0 ]; then
+ echo "✗ Error: Cannot calculate previous version for ${OLD_VERSION}"
+ exit 1
+fi
+PREV_VERSION="${MAJOR}.${PREV_MINOR}"
-find "$RELEASES_DIR" -type f -name "*${OLD_VERSION}*.json" 2>/dev/null | while read -r src_file; do
+# Use word-boundary pattern to avoid partial matches
+find "$RELEASES_DIR" -type f -name "*-${OLD_VERSION}.json" 2>/dev/null | while read -r src_file; do
dst_file="${src_file//${OLD_VERSION}/${NEW_VERSION}}"
[ -f "$dst_file" ] && continue
echo " $(basename "$src_file") → $(basename "$dst_file")"
cp "$src_file" "$dst_file"
# Bump version strings inside the file
- sed -i "s/${OLD_ESCAPED}/${NEW_VERSION}/g" "$dst_file"
- sed -i "s/${PREV_ESCAPED}/${CURR_VERSION}/g" "$dst_file"
+ sed -i.bak "s/${OLD_ESCAPED}/${NEW_VERSION}/g" "$dst_file" && rm -f "${dst_file}.bak"
+ sed -i.bak "s/${PREV_ESCAPED}/${CURR_VERSION}/g" "$dst_file" && rm -f "${dst_file}.bak"
doneAlso validate that RELEASES_DIR exists at line 17.
| cd "${CI_TOOLS_REPO}" | ||
|
|
||
| # Build the tool if not already built | ||
| echo "Building generated-release-gating-jobs..." | ||
| go build ./cmd/branchingconfigmanagers/generated-release-gating-jobs | ||
|
|
||
| # Run the bumper using Sippy config | ||
| echo "" | ||
| echo "Running generated-release-gating-jobs bumper..." | ||
| ./generated-release-gating-jobs \ | ||
| --current-release="${OLD_VERSION}" \ | ||
| --release-repo="${RELEASE_REPO}" \ | ||
| --sippy-config="${SIPPY_CONFIG}" \ | ||
| --interval=168 | ||
|
|
||
| echo "" | ||
| echo "==========================================" | ||
| echo "✓ Release gating jobs updated!" | ||
| echo "==========================================" |
There was a problem hiding this comment.
Missing Go availability check and hardcoded interval should be parameterizable.
Two issues in this section:
- Go dependency (Line 28): The
go buildwill fail cryptically if Go is not installed. Add a prerequisite check. - Hardcoded interval (Line 37): The
--interval=168is hardcoded (presumably representing 168 hours/1 week). This should either be a parameter to the script or documented as to why it's always fixed at this value. - Relative path (Line 33): The binary path
./generated-release-gating-jobsis relative to the current directory. Consider using an absolute path or storing the path in a variable for clarity.
Apply this diff:
# Add Go check after set -euo pipefail:
-set -euo pipefail
+set -euo pipefail
+
+if ! command -v go &> /dev/null; then
+ echo "✗ Error: 'go' not found in PATH"
+ exit 1
+fi
# Store binary path:
cd "${CI_TOOLS_REPO}"
# Build the tool if not already built
echo "Building generated-release-gating-jobs..."
-go build ./cmd/branchingconfigmanagers/generated-release-gating-jobs
+go build -o generated-release-gating-jobs-bin ./cmd/branchingconfigmanagers/generated-release-gating-jobs
# Run the bumper using Sippy config
echo ""
echo "Running generated-release-gating-jobs bumper..."
-./generated-release-gating-jobs \
+INTERVAL="${INTERVAL:-168}" # Allow override, default 168 hours
+./generated-release-gating-jobs-bin \
--current-release="${OLD_VERSION}" \
--release-repo="${RELEASE_REPO}" \
--sippy-config="${SIPPY_CONFIG}" \
- --interval=168
+ --interval="${INTERVAL}"Alternatively, if --interval=168 is intentionally fixed, document why in a comment.
🤖 Prompt for AI Agents
In .claude/.claude-plugin/skills/update-release-gating-jobs.sh around lines
24-42, add a runtime check for Go immediately after the existing set -euo
pipefail to fail fast with a clear message if go is missing; change the go build
invocation to write a named output binary (store that path in a variable, e.g.,
BIN_PATH) instead of relying on a relative executable name; introduce an
INTERVAL variable with a default (e.g., INTERVAL="${INTERVAL:-168}") and pass
that variable to the --interval flag so callers can override it; and use the
BIN_PATH variable when invoking the generated-release-gating-jobs binary (or add
a short comment if the 168-hour fixed interval must remain).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Prucek
force-pushed
the
ai-pre-branching
branch
from
9fafc9e to
c0f884c
Compare
|
/override ci/prow/e2e |
|
@Prucek: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
@Prucek: The following test failed, say
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. |
2 participants
Running this plugin from the claude cli will result in an automated pre-branching. And it should create a PR similar to this one: openshift/release#71837