Add Claude Code plugins for OpenShift release branching automation #4875
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughIntroduces a comprehensive OpenShift release branching automation framework comprising a plugin manifest, 40+ shell scripts organized into branching-day, post-branching, and common skill categories, and orchestration documentation for two workflows: branching-day and post-branching. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (15)
.claude/.claude-plugin/skills/branching-day/update-tide-and-infra-jobs.sh-14-22 (1)
14-22: Fragile sed range patterns.Both sed operations use
/name: <job-name>/,/secretName:/as the range delimiter. This assumes:
- The
secretName:field consistently marks the end of the job's args section.- No other jobs with similar names appear before the target job.
If the YAML structure changes (e.g.,
secretNameis removed or moved), the sed command could fail silently or update the wrong section.🔎 More robust approach
Consider using
yq(YAML query tool) for safer YAML editing:yq eval ' (.[] | select(.name == "periodic-openshift-release-merge-blockers") | .spec.containers[0].args[] | select(. == "--current-release='$CURRENT'")) = "--current-release='$FUTURE'" | (.[] | select(.name == "periodic-openshift-release-merge-blockers") | .spec.containers[0].args[] | select(. == "--future-release='$CURRENT'")) = "--future-release='$FUTURE'" ' -i "$INFRA"Alternatively, document the assumption and add verification that the pattern matches exactly once.
.claude/.claude-plugin/skills/branching-day/verify-etcd-config.sh-34-39 (1)
34-39: Apply consistent fix to other fragile grep patterns.The same fragility exists in:
- Lines 34-39:
grep -A 5 "without backport-risk-assessed"- Lines 42-47:
grep -A 10 "excludedBranches:"Both use fixed line counts that could break with YAML structure changes.
🔎 Consistent fixes
For development branch verification:
-if grep -A 5 "without backport-risk-assessed" "$ETCD_CONFIG" | grep -q "openshift-${FUTURE_RELEASE}"; then +if awk '/without backport-risk-assessed/,/excludedBranches:/ {print}' "$ETCD_CONFIG" | \ + grep -q "openshift-${FUTURE_RELEASE}"; then echo "✓ openshift-${FUTURE_RELEASE} set as development branch"For excluded branches verification:
-if grep -A 10 "excludedBranches:" "$ETCD_CONFIG" | grep -q "openshift-${FUTURE_RELEASE}"; then +if awk '/excludedBranches:/,/labels:/ {print}' "$ETCD_CONFIG" | \ + grep -q "openshift-${FUTURE_RELEASE}"; then echo "✓ openshift-${FUTURE_RELEASE} added to excluded branches"Also applies to: 42-47
.claude/.claude-plugin/skills/post-branching/verify-fast-forward-job.sh-38-44 (1)
38-44: Apply the same fix to the old value verification.The same fragility issue exists here with
grep -A 10.🔎 Consistent fix
-if grep -A 10 "name: periodic-openshift-release-fast-forward" "$INFRA_PERIODICS" | \ - grep -q -- "--future-release=${XY_PLUS_1}"; then +if awk '/name: periodic-openshift-release-fast-forward/,/^[^ ]/ {print}' "$INFRA_PERIODICS" | \ + grep -q -- "--future-release=${XY_PLUS_1}"; then echo "✗ FAILED: Old value ${XY_PLUS_1} still present" VERIFICATION_FAILED=1 else echo "✓ Old value ${XY_PLUS_1} removed" fi.claude/.claude-plugin/skills/post-branching/verify-fast-forward-job.sh-29-35 (1)
29-35: Fragile grep pattern with fixed line lookahead.Using
grep -A 10assumes the--future-releaseflag appears within 10 lines after the job name. If the job definition expands beyond 10 lines, this verification could produce false negatives.Consider using a more robust approach:
- Extract the entire job block using a YAML parser.
- Use
awkorsedto capture the complete job definition between this job and the next job marker.🔎 Alternative approach using awk
-if grep -A 10 "name: periodic-openshift-release-fast-forward" "$INFRA_PERIODICS" | \ - grep -q -- "--future-release=${XY_PLUS_2}"; then +if awk '/name: periodic-openshift-release-fast-forward/,/^[^ ]/ {print}' "$INFRA_PERIODICS" | \ + grep -q -- "--future-release=${XY_PLUS_2}"; then echo "✓ --future-release updated to ${XY_PLUS_2}" else echo "✗ FAILED: --future-release not updated correctly" VERIFICATION_FAILED=1 fiThis extracts all lines from the job name until the next non-indented line (next top-level key), ensuring the entire job definition is captured.
.claude/.claude-plugin/skills/branching-day/update-etcd-config.sh-12-16 (1)
12-16: Independent sed operations with no error checking.The three
sedoperations execute independently. If one fails (e.g., pattern not found), the subsequent operations still run, potentially leaving the config in an inconsistent state.With
set -e, the script will exit ifsedreturns non-zero, butsedreturns 0 even when no substitutions are made.🔎 Add verification after each operation
sed -i "/- openshift-4.20$/a\ - openshift-${CURRENT}" "$ETCD" +if ! grep -q "openshift-${CURRENT}" "$ETCD"; then + echo "Error: Failed to add ${CURRENT} to older releases" + exit 1 +fi + sed -i "s/- openshift-${CURRENT}$/- openshift-${FUTURE}/" "$ETCD" +if ! grep -q "openshift-${FUTURE}" "$ETCD"; then + echo "Error: Failed to update development branch to ${FUTURE}" + exit 1 +fi + sed -i "/excludedBranches:/,/labels:/ { /- openshift-${CURRENT}$/a\ - openshift-${FUTURE} }" "$ETCD" +if ! grep -A 10 "excludedBranches:" "$ETCD" | grep -q "openshift-${FUTURE}"; then + echo "Error: Failed to add ${FUTURE} to excluded branches" + exit 1 +fi.claude/.claude-plugin/skills/branching-day/verify-etcd-config.sh-26-31 (1)
26-31: Fragile grep pattern with fixed line context.Using
grep -B 5 "backport-risk-assessed"assumes theopenshift-${CURRENT_RELEASE}entry appears within 5 lines before the "backport-risk-assessed" marker. If the YAML structure changes or more entries are added, this could produce false negatives.🔎 More robust verification
-if grep -B 5 "backport-risk-assessed" "$ETCD_CONFIG" | grep -q "openshift-${CURRENT_RELEASE}"; then +# Find the "older releases" section and verify current release is present +if awk '/without backport-risk-assessed/,/labels:/ {exit} {print}' "$ETCD_CONFIG" | \ + grep -q "openshift-${CURRENT_RELEASE}"; then echo "✓ openshift-${CURRENT_RELEASE} added to older releases query" else echo "✗ FAILED: openshift-${CURRENT_RELEASE} not found in older releases query" VERIFICATION_FAILED=1 fiThis extracts everything before "without backport-risk-assessed" (the development section), which should contain the older releases.
.claude/.claude-plugin/skills/branching-day/commit-image-mirroring-pr.sh-9-10 (1)
9-10: Non-atomic commit sequence risks inconsistent state.If the first
git commitsucceeds but the second fails, the repository will be left in an inconsistent state with only one of the two expected commits.Consider:
- Combining both changes into a single commit.
- Adding error handling between commits.
- Using a transaction-like pattern (staging all changes, then single commit).
🔎 Options to address
Option 1: Single commit
-git add core-services/image-mirroring/openshift/_config.yaml && git commit -m "bump image-mirroring config" -git add -A && git commit -m "make openshift-image-mirror-mappings" +git add core-services/image-mirroring/openshift/_config.yaml +git add -A +git commit -m "bump image-mirroring config + +make openshift-image-mirror-mappings"Option 2: Error handling
git add core-services/image-mirroring/openshift/_config.yaml && git commit -m "bump image-mirroring config" +if [ $? -ne 0 ]; then + echo "Error: First commit failed" + exit 1 +fi git add -A && git commit -m "make openshift-image-mirror-mappings"Note: With
set -e, the script already exits on first failure, but making this explicit improves clarity..claude/.claude-plugin/skills/post-branching/verify-fast-forward-job.sh-29-44 (1)
29-44: Verify job existence before checking its flags.If the
periodic-openshift-release-fast-forwardjob is missing entirely from the file, both grep checks will pass (returning empty results), leading to a false positive verification.🔎 Add job existence check
VERIFICATION_FAILED=0 +# Verify the job exists +if ! grep -q "name: periodic-openshift-release-fast-forward" "$INFRA_PERIODICS"; then + echo "✗ FAILED: periodic-openshift-release-fast-forward job not found" + VERIFICATION_FAILED=1 + echo "==========================================" + echo "Verification FAILED" + exit 1 +fi + # Verify the update was successfulCommittable suggestion skipped: line range outside the PR's diff.
.claude/.claude-plugin/skills/post-branching/update-merge-blockers-job.sh-12-16 (1)
12-16: Verify that substitution succeeds.The script doesn't check whether
sedactually found and replaced the pattern. If the target pattern (--future-release=${XY_PLUS_1}) doesn't exist in the file,sedwill silently succeed without making changes, but the script will still print a success message.🔎 Proposed verification
+# Verify pattern exists before substitution +if ! grep -q -- "--future-release=${XY_PLUS_1}" "$INFRA"; then + echo "Error: Pattern --future-release=${XY_PLUS_1} not found in infra-periodics.yaml" + exit 1 +fi + sed -i "/name: periodic-openshift-release-merge-blockers$/,/name:/ { s/--future-release=${XY_PLUS_1}/--future-release=${XY_PLUS_2}/ }" "$INFRA" +# Verify substitution succeeded +if ! grep -q -- "--future-release=${XY_PLUS_2}" "$INFRA"; then + echo "Error: Substitution failed" + exit 1 +fi + echo "✓ merge-blockers: $XY_PLUS_1 → $XY_PLUS_2".claude/.claude-plugin/skills/branching-day/update-image-mirroring.sh-9-13 (1)
9-13: Script continues after manual edit warning.The script prints "MANUAL EDIT REQUIRED" but doesn't exit afterward, implying successful completion. This creates ambiguity: should the automation wait for user action, or should it proceed?
Consider either:
- Exiting with a specific code to signal manual intervention is needed:
echo "✓ Manual edit instructions displayed" exit 0
- Or prompting the user and waiting for confirmation before continuing the workflow.
.claude/.claude-plugin/skills/post-branching/verify-imagestream-tags.sh-15-16 (1)
15-16:jqmay returnnullinstead of triggering fallback.If an ImageStream exists but has no
.status.tagsfield,jq '.status.tags | length'outputsnull(not an error), so the|| echo "0"fallback is never triggered. This causes the comparison at line 21 to fail with a non-numeric value.🔎 Proposed fix
-OLD_COUNT=$(oc --context=app.ci get is "$OLD_IS" -n "$NAMESPACE" -o json 2>/dev/null | jq '.status.tags | length' || echo "0") -NEW_COUNT=$(oc --context=app.ci get is "$NEW_IS" -n "$NAMESPACE" -o json 2>/dev/null | jq '.status.tags | length' || echo "0") +OLD_COUNT=$(oc --context=app.ci get is "$OLD_IS" -n "$NAMESPACE" -o json 2>/dev/null | jq '.status.tags // [] | length' || echo "0") +NEW_COUNT=$(oc --context=app.ci get is "$NEW_IS" -n "$NAMESPACE" -o json 2>/dev/null | jq '.status.tags // [] | length' || echo "0")This uses jq's alternative operator (
//) to treat null/missing as an empty array before computing length..claude/.claude-plugin/skills/post-branching/update-auto-config-brancher-x-y-plus-2.sh-12-14 (1)
12-14: Sed range pattern may be fragile.The pattern
/name: periodic-prow-auto-config-brancher/,/name:/assumes a specific YAML structure where the nextname:marks the end of the job definition. This could break if:
- The job definition contains nested
name:fields- Comments or other YAML structures are added
- The job order changes
🔎 More robust alternatives
Option 1: Use
yqfor proper YAML parsing:yq -i '.periodics[] |= ( select(.name == "periodic-prow-auto-config-brancher") | .spec.containers[0].args[] |= sub("--future-release='"${XY_PLUS_1}"'"; "--future-release='"${XY_PLUS_2}"'") )' "$INFRA"Option 2: Use a more specific sed range (match to next top-level entry):
sed -i "/name: periodic-prow-auto-config-brancher/,/^- name:/ { s/--future-release=${XY_PLUS_1}/--future-release=${XY_PLUS_2}/ }" "$INFRA".claude/.claude-plugin/skills/post-branching/update-image-mirroring-x-y-plus-2.sh-18-30 (1)
18-30: YAML manipulation is fragile.The awk script manipulates YAML based on string patterns and indentation assumptions. This is brittle and will break if:
- Indentation changes (spaces vs tabs)
- YAML formatting changes
- Comments are added between entries
- Key ordering changes
🔎 Recommend using `yq` for YAML manipulation
add_version() { local prefix="$1" after="$2" new="$3" local after_key="${prefix}${after}" new_key="${prefix}${new}" # Check if key exists if yq -e ".\"${new_key}\"" "$CONFIG" >/dev/null 2>&1; then return 0 fi # Insert new key after the specified key yq -i ".\"${new_key}\" = [\"${new}\", \"${new}.0\"]" "$CONFIG" }Using
yqprovides:
- Proper YAML parsing and serialization
- Immunity to whitespace changes
- Better error handling
- More maintainable code
If
yqis not available, add it as a dependency in the tooling setup..claude/.claude-plugin/skills/common/build-branching-tooling.sh-12-22 (1)
12-22: Preserve build output for debugging.Silencing all make output (
>/dev/null 2>&1) hides compilation errors, warnings, and other diagnostic information that would be valuable for troubleshooting build failures.🔎 Proposed fix to preserve error output
for tool in "${TOOLS[@]}"; do - if make install WHAT="cmd/$tool" >/dev/null 2>&1; then + if make install WHAT="cmd/$tool" 2>&1 | grep -v "^make\["; then echo "✓ $tool" else - echo "✗ $tool" + echo "✗ $tool (build failed - see output above)" FAILED=1 fi doneAlternatively, redirect only stdout to preserve errors:
- if make install WHAT="cmd/$tool" >/dev/null 2>&1; then + if make install WHAT="cmd/$tool" >/dev/null; then.claude/.claude-plugin/skills/post-branching/create-config-jobs-x-y-plus-2.sh-33-44 (1)
33-44: Replace yaml.full_load with yaml.safe_load.
yaml.full_load()can execute arbitrary Python code embedded in YAML documents, which is a security risk. Useyaml.safe_load()instead, which only constructs simple Python objects.While these are internal CI configuration files, using
safe_load()is a security best practice that prevents potential code execution vulnerabilities.🔎 Proposed fix
cat <<'EOF' >/tmp/filter.py #!/usr/bin/env python3 import yaml, sys with open(sys.argv[1]) as f: - all = yaml.full_load(f) + all = yaml.safe_load(f) for t in ("presubmits", "postsubmits"): for repo in all.get(t, {}): all[t][repo] = [j for j in all.get(t, {}).get(repo, []) if j.get("agent", "") == "kubernetes"] with open(sys.argv[1], 'w') as f: yaml.dump(all, f, default_flow_style=False) EOF
🟡 Minor comments (6)
.claude/.claude-plugin/skills/branching-day/verify-infra-periodics.sh-7-15 (1)
7-15: Remove unused variable.
CURRENT_RELEASEis assigned but never used in the verification logic. This appears to be a remnant from earlier development.🔎 Proposed fix
-if [ "$#" -ne 3 ]; then - echo "Usage: $0 <current-release> <future-release> <release-repo-path>" +if [ "$#" -ne 2 ]; then + echo "Usage: $0 <future-release> <release-repo-path>" echo "Example: $0 4.21 4.22 ../release" exit 1 fi -CURRENT_RELEASE="$1" -FUTURE_RELEASE="$2" -RELEASE_REPO="$3" +FUTURE_RELEASE="$1" +RELEASE_REPO="$2"Alternatively, if
CURRENT_RELEASEwill be needed in future verification steps, add a comment explaining its purpose..claude/.claude-plugin/skills/post-branching/mirror-imagestreams-to-quay.sh-30-36 (1)
30-36: Off-by-one in retry message display.Line 35 displays
$retries/$((MAX_RETRIES - 1)), which shows "Retry 1/2" whenMAX_RETRIES=3. This is confusing since there are 3 total attempts. The message should reflect the actual retry count.🔎 Proposed fix
- [ $retries -lt $MAX_RETRIES ] && echo "Retry $retries/$((MAX_RETRIES - 1)) for $namespace/$name..." && sleep 5 + [ $retries -lt $MAX_RETRIES ] && echo "Retry $retries for $namespace/$name (attempt $((retries + 1))/$MAX_RETRIES)..." && sleep 5.claude/.claude-plugin/skills/post-branching/verify-image-mirroring-x-y-plus-2.sh-13-13 (1)
13-13: Remove unused variable.The
XY_PLUS_1variable is assigned but never used in the verification logic. OnlyXY_PLUS_2is referenced in the checks.🔎 Proposed fix
-XY_PLUS_1="$1" XY_PLUS_2="$2" RELEASE_REPO="$3"Also update the usage message to reflect that only two version arguments are needed:
- echo "Usage: $0 <x.y+1> <x.y+2> <release-repo-path>" - echo "Example: $0 4.22 4.23 ../release" + echo "Usage: $0 <x.y+2> <release-repo-path>" + echo "Example: $0 4.23 ../release"Committable suggestion skipped: line range outside the PR's diff.
.claude/.claude-plugin/skills/post-branching/verify-merge-blockers-job.sh-13-13 (1)
13-13: Remove unused variable.The
CURRENT_RELEASEvariable is assigned but never used. OnlyXY_PLUS_1(derived from it) is referenced in the verification logic.🔎 Proposed fix
-CURRENT_RELEASE="$1" XY_PLUS_2="$2" RELEASE_REPO="$3" # Calculate X.Y+1 -XY_PLUS_1=$(echo "$CURRENT_RELEASE" | awk -F. '{printf "%d.%d", $1, $2+1}') +XY_PLUS_1=$(echo "$1" | awk -F. '{printf "%d.%d", $1, $2+1}').claude/.claude-plugin/skills/post-branching/verify-jira-validation.sh-13-13 (1)
13-13: Remove unused variable.The
CURRENT_RELEASEvariable is assigned but never used. OnlyXY_PLUS_2andXY_PLUS_3(derived from it) are referenced in the verification logic.🔎 Proposed fix
-CURRENT_RELEASE="$1" XY_PLUS_2="$2" RELEASE_REPO="$3"Committable suggestion skipped: line range outside the PR's diff.
.claude/.claude-plugin/skills/post-branching/create-config-jobs-x-y-plus-2.sh-6-11 (1)
6-11: Reorder directory validation before cd command.The directory existence check on line 11 is ineffective because:
- If
cd "$3"on line 9 succeeds, the directory exists, making line 11 redundant.- If
cd "$3"fails (directory doesn't exist), the script exits immediately due toset -e, and line 11 never executes.Move the validation before the cd command for a clearer error message.
🔎 Proposed fix
CURRENT="$1" NEXT="$2" CI_TOOLS=$(pwd) -RELEASE=$(cd "$3" && pwd) + +[ ! -d "$3" ] && echo "Error: Release repo not found" && exit 1 +RELEASE=$(cd "$3" && pwd) -[ ! -d "$RELEASE" ] && echo "Error: Release repo not found" && exit 1 -
🧹 Nitpick comments (33)
.claude/.claude-plugin/skills/branching-day/update-tide-and-infra-jobs.sh (2)
1-24: Misleading script name.The script is named
update-tide-and-infra-jobs.shbut it updatesperiodic-openshift-release-merge-blockersandperiodic-ocp-build-data-enforcerjobs, neither of which are tide-related. The tide configuration is updated separately byupdate-etcd-config.shandrun-tide-config-manager.sh.Consider renaming to
update-merge-blockers-and-enforcer.shor similar.
8-9: Version extraction assumes X.Y format.The minor version extraction
${CURRENT#*.}works forX.Yformat but will fail forX.Y.Zformat (e.g.,4.21.1would extract21.1instead of21).While OpenShift currently uses X.Y versioning, this assumption should be documented or made more robust.
🔎 More robust version extraction
-CURRENT_MINOR="${CURRENT#*.}" -FUTURE_MINOR="${FUTURE#*.}" +CURRENT_MINOR=$(echo "$CURRENT" | cut -d. -f2) +FUTURE_MINOR=$(echo "$FUTURE" | cut -d. -f2)Or add validation:
+# Validate X.Y format +if [[ ! "$CURRENT" =~ ^[0-9]+\.[0-9]+$ ]] || [[ ! "$FUTURE" =~ ^[0-9]+\.[0-9]+$ ]]; then + echo "Error: Versions must be in X.Y format" + exit 1 +fi + CURRENT_MINOR="${CURRENT#*.}" FUTURE_MINOR="${FUTURE#*.}".claude/.claude-plugin/skills/branching-day/commit-image-mirroring-pr.sh (2)
9-9: Missing file existence verification.The script attempts to add
core-services/image-mirroring/openshift/_config.yamlwithout verifying it exists. If the file doesn't exist,git addwill fail, but the error message won't indicate the root cause clearly.🔎 Add file existence check
+if [ ! -f "core-services/image-mirroring/openshift/_config.yaml" ]; then + echo "Error: _config.yaml not found" + exit 1 +fi + git add core-services/image-mirroring/openshift/_config.yaml && git commit -m "bump image-mirroring config"
7-7: Improve error message clarity.The error message "Error: Wrong branch" doesn't indicate which branch is expected or which branch is current, making debugging harder.
🔎 More descriptive error message
-[ "$(git branch --show-current)" != "image-mirroring-$2" ] && echo "Error: Wrong branch" && exit 1 +CURRENT_BRANCH=$(git branch --show-current) +EXPECTED_BRANCH="image-mirroring-$2" +if [ "$CURRENT_BRANCH" != "$EXPECTED_BRANCH" ]; then + echo "Error: Wrong branch. Expected: $EXPECTED_BRANCH, Current: $CURRENT_BRANCH" + exit 1 +fi.claude/.claude-plugin/skills/branching-day/run-tide-config-manager.sh (2)
9-15: Missing tide-config-manager availability check.The script assumes
tide-config-manageris available in PATH and accepts the provided flags. If the tool is not installed or has a different interface, the script will fail with an unclear error message.🔎 Add tool availability check
RELEASE_REPO=$(cd "$2" && pwd) +# Verify tide-config-manager is available +if ! command -v tide-config-manager &> /dev/null; then + echo "Error: tide-config-manager not found in PATH" + exit 1 +fi + tide-config-manager \
9-15: No verification of tide-config-manager output.The script prints a success message regardless of whether
tide-config-manageractually modified any files or completed successfully. Withset -e, the script will exit if the tool returns non-zero, but it doesn't verify that expected changes were made.Consider:
- Adding a companion verify script call (like
verify-tide-config-manager.shin this PR).- Checking for specific output patterns.
- Verifying that expected files were modified.
.claude/.claude-plugin/skills/post-branching/commit-merge-blockers-pr.sh (1)
9-13: Silent skip if no changes might be unexpected.The pattern
git diff --staged --quiet || git commitwill silently skip the commit if there are no staged changes. Depending on the workflow, this might be:
- Expected behavior (no changes to commit is fine).
- Unexpected behavior (if changes were expected, silent skip could hide an earlier failure).
Consider adding explicit feedback:
🔎 Explicit no-changes handling
-git add ci-operator/jobs/infra-periodics.yaml && \ - git diff --staged --quiet || \ - git commit -m "Update merge blockers job for $1 +git add ci-operator/jobs/infra-periodics.yaml +if git diff --staged --quiet; then + echo "✓ No changes to commit" +else + git commit -m "Update merge blockers job for $1 Configure periodic-openshift-release-merge-blockers to track $1 branches." +fi.claude/.claude-plugin/skills/branching-day/verify-tide-config-manager.sh (1)
26-34: Verification only checks for modifications, not correctness.The script verifies that
core-services/prow/02_config/was modified but doesn't validate:
- What changes were made.
- Whether the changes are correct for the current release.
- Whether required changes are missing.
This could pass for incorrect modifications or miss important validation failures.
Consider adding specific checks:
- Verify expected files were modified (e.g., specific org/repo config files).
- Check for presence of release-specific markers.
- Validate YAML syntax with
yqor similar tool.Example:
# Verify specific expected changes EXPECTED_FILES=( "core-services/prow/02_config/openshift/etcd/_prowconfig.yaml" # Add other expected files ) for file in "${EXPECTED_FILES[@]}"; do if ! git diff --name-only "$file" | grep -q "$file"; then echo "✗ Expected file not modified: $file" VERIFICATION_FAILED=1 fi done.claude/.claude-plugin/skills/branching-day/verify-etcd-config.sh (1)
26-47: Missing verification that old values were removed.The script verifies that new values (CURRENT and FUTURE) are present in the expected locations but doesn't verify that old values were removed. For example:
- Was the previous development branch moved to the older releases section?
- Were outdated versions removed from excluded branches?
This could allow accumulation of stale configuration.
🔎 Add removal verification
+# Calculate the previous development release (FUTURE - 1) +PREVIOUS_DEV=$(echo "$FUTURE_RELEASE" | awk -F. '{printf "%d.%d", $1, $2-1}') + +# Verify previous dev release is no longer in development section +if awk '/without backport-risk-assessed/,/excludedBranches:/ {print}' "$ETCD_CONFIG" | \ + grep -q "openshift-${PREVIOUS_DEV}"; then + echo "✗ WARNING: Old development release ${PREVIOUS_DEV} still in development section" + # Don't fail, but warn +fi + echo "==========================================".claude/.claude-plugin/skills/post-branching/update-merge-blockers-job.sh (1)
4-8: Add input format validation.The script assumes arguments are in
X.Yformat but doesn't validate this. Malformed input like4.21.0orabccould lead to incorrect version calculations or unexpected behavior.🔎 Proposed validation
[ "$#" -ne 3 ] && echo "Usage: $0 <current> <x.y+2> <release-repo>" && exit 1 +# Validate version format (X.Y) +if ! echo "$1" | grep -qE '^[0-9]+\.[0-9]+$' || ! echo "$2" | grep -qE '^[0-9]+\.[0-9]+$'; then + echo "Error: Version arguments must be in X.Y format (e.g., 4.21)" + exit 1 +fi + XY_PLUS_2="$2".claude/.claude-plugin/skills/branching-day/verify-tide-and-infra-jobs.sh (2)
13-18: Remove or use the CURRENT_RELEASE variable.
CURRENT_RELEASEis assigned but never used in the script. All verification checks useFUTURE_RELEASEinstead. If this variable isn't needed, remove it to avoid confusion; otherwise, verify whether it should be used in one of the checks.Based on learnings from Shellcheck warning SC2034.
37-44: Consider scoping the grep to the specific job.The verification searches the entire file for
base_ref: openshift-${FUTURE_RELEASE}and--minor=${FUTURE_MINOR}without scoping to theperiodic-ocp-build-data-enforcerjob context. If these patterns appear in other jobs, this could produce false positives.🔎 More precise verification
# Verify periodic-ocp-build-data-enforcer was updated -if grep -q "base_ref: openshift-${FUTURE_RELEASE}" "$INFRA_PERIODICS" && \ - grep -q -- "--minor=${FUTURE_MINOR}" "$INFRA_PERIODICS"; then +if grep -A 20 "name: periodic-ocp-build-data-enforcer" "$INFRA_PERIODICS" | \ + grep -q "base_ref: openshift-${FUTURE_RELEASE}" && \ + grep -A 20 "name: periodic-ocp-build-data-enforcer" "$INFRA_PERIODICS" | \ + grep -q -- "--minor=${FUTURE_MINOR}"; then echo "✓ periodic-ocp-build-data-enforcer updated correctly" else echo "✗ FAILED: periodic-ocp-build-data-enforcer not updated correctly".claude/.claude-plugin/skills/post-branching/verify-config-jobs-x-y-plus-2.sh (1)
13-15: Remove unused variable.
XY_PLUS_1is assigned but never used in the script. All verification checks useNEXTinstead. Remove this assignment to avoid confusion.Based on learnings from Shellcheck warning SC2034.
🔎 Proposed fix
-XY_PLUS_1="$1" NEXT="$2" RELEASE_REPO="$3".claude/.claude-plugin/skills/branching-day/commit-tide-config-pr.sh (1)
13-14: Simplify conditional commit syntax.The conditional etcd commit logic is correct but uses an unusual subshell grouping. This can be simplified for clarity.
🔎 Proposed simplification
-git diff --quiet core-services/prow/02_config/openshift/etcd/_prowconfig.yaml || \ - (git add core-services/prow/02_config/openshift/etcd/_prowconfig.yaml && git commit -m "etcd manual change") +if ! git diff --quiet core-services/prow/02_config/openshift/etcd/_prowconfig.yaml; then + git add core-services/prow/02_config/openshift/etcd/_prowconfig.yaml + git commit -m "etcd manual change" +fi.claude/.claude-plugin/skills/post-branching/update-jira-validation-post-branching.sh (2)
6-9: Add input format validation.The version calculations assume the input is in
X.Yformat but don't validate this. Malformed input could produce incorrect version numbers.🔎 Proposed validation
XY_PLUS_2="$2" +# Validate version format +if ! echo "$XY_PLUS_2" | grep -qE '^[0-9]+\.[0-9]+$'; then + echo "Error: Version must be in X.Y format (e.g., 4.22)" + exit 1 +fi XY_PLUS_3=$(echo "$XY_PLUS_2" | awk -F. '{printf "%d.%d", $1, $2+1}') XY_PLUS_1=$(echo "$XY_PLUS_2" | awk -F. '{printf "%d.%d", $1, $2-1}')
34-49: Add cleanup trap for temporary files.The script creates temporary files but doesn't have cleanup logic if the script exits unexpectedly. With
set -euo pipefail, any failure would leave.tmpand.tmp2files behind.🔎 Proposed cleanup trap
JIRA="$3/core-services/jira-lifecycle-plugin/config.yaml" [ ! -f "$JIRA" ] && echo "Error: Jira config not found" && exit 1 grep -q "openshift-${XY_PLUS_2}:" "$JIRA" && echo "Warning: Already exists, skipping" && exit 0 +# Setup cleanup trap +trap 'rm -f "${JIRA}.tmp" "${JIRA}.tmp2"' EXIT ERR INT TERM + OPENSHIFT_STANZA=" openshift-${XY_PLUS_2}:.claude/.claude-plugin/skills/common/trigger-periodic-jobs.sh (3)
34-53: Consider more robust JSON response parsing.The function extracts the execution ID using
grep -oandcut, which is fragile and could break if the API response format changes. Consider usingjqfor reliable JSON parsing.🔎 Proposed improvement with jq
trigger_job() { local job_name="$1" - local payload=$(cat <<EOF + local payload + payload=$(cat <<EOF { "job_execution_type": "1", "pod_spec_options": { "envs": {}, "job_name": "$job_name" } } EOF ) curl -s -X POST \ -H "Authorization: Bearer $TOKEN" \ -H "Content-Type: application/json" \ -d "$payload" \ "$GANGWAY_URL/v1/executions" \ - | grep -o '"id":"[^"]*"' | cut -d'"' -f4 + | jq -r '.id // empty' }Note: The Shellcheck SC2155 warning about declare and assign can be safely ignored for heredoc usage as shown here, but I've separated the declaration for better style.
55-60: Use jq for reliable JSON parsing.Similar to
trigger_job, this function should usejqfor more reliable JSON parsing instead ofgrepandcut.🔎 Proposed improvement
check_status() { curl -s -X GET \ -H "Authorization: Bearer $TOKEN" \ "$GANGWAY_URL/v1/executions/$1" \ - | grep -o '"job_status":"[^"]*"' | cut -d'"' -f4 + | jq -r '.job_status // empty' }
62-90: LGTM with minor style suggestion.The polling logic is well-implemented with appropriate timeout handling and status checking. The function correctly returns exit codes to signal success or failure.
For better style, consider separating the status variable declaration and assignment to address Shellcheck SC2155:
while [ $elapsed -lt $max_wait ]; do - local status=$(check_status "$execution_id") + local status + status=$(check_status "$execution_id").claude/.claude-plugin/skills/common/get-current-release.sh (2)
9-11: Consider simplifying the version extraction.The multi-stage pipeline works but is complex. Consider using a more targeted grep pattern or
awkto reduce the number of pipeline stages.🔎 Proposed simplification
-VERSION=$(grep -A 20 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | grep "current-release=" | head -1 | sed 's/.*--current-release=//' | awk '{print $1}') +VERSION=$(grep -A 20 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \ + grep -oP '(?<=--current-release=)[0-9.]+' | head -1)Or using
awkonly:-VERSION=$(grep -A 20 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | grep "current-release=" | head -1 | sed 's/.*--current-release=//' | awk '{print $1}') +VERSION=$(awk '/name: periodic-prow-auto-config-brancher/,/^[^ ]/ { + if (match($0, /--current-release=([0-9.]+)/, m)) { print m[1]; exit } +}' "$INFRA_PERIODICS")
13-17: Add HTTP error handling for API call.The script uses
curl -swhich suppresses errors. If the API is unreachable or returns a non-200 status, the script might not detect the failure untiljqfails on invalid JSON. Consider addingcurl --failor checking the HTTP status code.🔎 Proposed improvement
NIGHTLY_API="https://amd64.ocp.releases.ci.openshift.org/api/v1/releasestream/${VERSION}.0-0.nightly/tags" -ACCEPTED_COUNT=$(curl -s "$NIGHTLY_API" | jq '[.tags[] | select(.phase == "Accepted")] | length') +ACCEPTED_COUNT=$(curl -sf "$NIGHTLY_API" | jq '[.tags[] | select(.phase == "Accepted")] | length' || echo "") -[ -z "$ACCEPTED_COUNT" ] && echo "Error: Failed to fetch nightly releases for $VERSION" && exit 1 +if [ -z "$ACCEPTED_COUNT" ]; then + echo "Error: Failed to fetch nightly releases for $VERSION" + echo "API URL: $NIGHTLY_API" + exit 1 +fi.claude/commands/post-branching.md (1)
119-119: Trailing space in argument may confuse users.The example shows
.(dot followed by space). While this may be intentional, it could confuse users. Consider clarifying in the documentation whether the trailing space is significant or should be omitted.🔎 Proposed clarification
-Invoke skill: `build-branching-tooling` with args: `. ` +Invoke skill: `build-branching-tooling` with args: `.` (current directory).claude/.claude-plugin/skills/branching-day/update-infra-periodics.sh (2)
14-18:sedrange pattern may match unintended blocks.The range
/name: periodic-prow-auto-config-brancher/,/name:/stops at the next occurrence ofname:, which could be a nested field or an adjacent job definition. This may cause incorrect replacements if the YAML structure contains othername:keys within the target block.Consider using
yq(a YAML-aware tool) instead ofsedfor safer, structure-aware updates:yq eval ' (.[] | select(.name == "periodic-prow-auto-config-brancher") | .spec.containers[].args[] | select(. == "--current-release='$CURRENT'")) = "--current-release='$FUTURE'" | (.[] | select(.name == "periodic-prow-auto-config-brancher") | .spec.containers[].args[] | select(. == "--future-release='$CURRENT'")) = "--future-release='$FUTURE'" ' -i "$INFRA"Alternatively, add a verification step after the
sedoperations to confirm the changes were applied correctly.
25-30:sedrange patterns assume rigid YAML indentation.Lines 25-30 use patterns like
/^ main:/,/^ [a-z]/that depend on exactly 2-space indentation. If the YAML is reformatted or uses a different indent style, these patterns will fail silently.Consider either:
- Using
yqfor structure-aware updates- Adding a verification script (similar to
verify-infra-periodics.shfor the branching-day workflow) to confirm the replacements succeeded.claude/.claude-plugin/skills/branching-day/verify-infra-periodics.sh (1)
35-42: Consider more robust YAML parsing.Using
grep -A 10assumes the--current-releaseflag appears within 10 lines of the job name. This could break if the YAML structure changes or if comments are added.💡 More robust alternatives
Option 1: Use
yqfor proper YAML parsing:if yq '.periodics[] | select(.name == "periodic-openshift-release-fast-forward") | .spec.containers[0].args[] | select(. == "--current-release='"${FUTURE_RELEASE}"'")' "$INFRA_PERIODICS" &>/dev/null; then echo "✓ periodic-openshift-release-fast-forward updated correctly" else echo "✗ FAILED: periodic-openshift-release-fast-forward not updated correctly" VERIFICATION_FAILED=1 fiOption 2: Use a more specific range (stop at next job):
if sed -n '/name: periodic-openshift-release-fast-forward/,/^ - name:/p' "$INFRA_PERIODICS" | \ grep -q -- "--current-release=${FUTURE_RELEASE}"; then.claude/.claude-plugin/skills/common/create-branch-from-master.sh (1)
1-14: Consider simplifying remote detection.Line 13 uses a subshell with grep to detect the remote, which works but is verbose.
💡 Simpler alternative
-REMOTE=$(git remote | grep -q "^upstream$" && echo "upstream" || echo "origin") +REMOTE="origin" +git remote | grep -q "^upstream$" && REMOTE="upstream"This separates the detection logic and is easier to read.
.claude/.claude-plugin/skills/post-branching/update-fast-forward-job.sh (1)
12-14: Consider adding verification of sed success.The script performs an in-place edit but doesn't verify that the substitution succeeded. For a fail-fast architecture, consider checking that the expected change was made.
Example verification approach
# After sed command if ! grep -q -- "--future-release=${XY_PLUS_2}" "$INFRA"; then echo "Error: Failed to update --future-release" exit 1 fi.claude/commands/branching-day.md (3)
108-108: Minor formatting: Remove spaces in code span.The code span
.on line 108 has spaces that could be removed for cleaner formatting.🔎 Proposed fix
-Invoke skill: `build-branching-tooling` with args: `. ` +Invoke skill: `build-branching-tooling` with args: `.`
186-186: Minor grammar: Add hyphen to compound adjective.The phrase "20 minute timeout" should be hyphenated as "20-minute timeout" when used as a compound adjective.
Based on coding guidelines.
🔎 Proposed fix
-- Report success when both complete (20 minute timeout) +- Report success when both complete (20-minute timeout)
221-221: Minor formatting: Wrap bare URL.The bare URL on line 221 should be wrapped in angle brackets per Markdown best practices.
🔎 Proposed fix
-**Example PR:** https://github.com/openshift/release/pull/59015 +**Example PR:** <https://github.com/openshift/release/pull/59015>.claude/.claude-plugin/skills/post-branching/create-config-jobs-x-y-plus-2.sh (3)
13-16: Consider quoting variable in command check.While not critical for these specific tool names, it's best practice to quote variables in shell expansions.
🔎 Proposed fix
cd "$CI_TOOLS" for tool in config-brancher ci-operator-config-mirror rpm-deps-mirroring-services; do - command -v $tool &>/dev/null || go install ./cmd/$tool + command -v "$tool" &>/dev/null || go install ./cmd/"$tool" done
30-31: Use more specific git add pattern.Using
git add .stages all changes in the repository, which could include unintended files. Consider being more specific about which files the rpm-deps-mirroring-services command modifies.🔎 If the tool only modifies known paths, use a specific pattern
For example, if rpm-deps-mirroring-services only modifies files under
core-services/, use:rpm-deps-mirroring-services --current-release "$CURRENT" --release-repo "$RELEASE" -git add . && git commit -m "[$NEXT] rpm dependency update" +git add core-services/ && git commit -m "[$NEXT] rpm dependency update"Verify which paths this tool modifies and adjust accordingly.
46-54: Consider adding comments to clarify the job processing logic.The loop performs several operations:
- Finds job files for the CURRENT release
- Locates the corresponding master/main variant
- Copies and adapts it to the NEXT release
- Filters out non-kubernetes agents
While the logic is sound, adding brief inline comments would improve maintainability for future developers.
📜 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 (41)
.claude/.claude-plugin/plugin.json(1 hunks).claude/.claude-plugin/skills/branching-day/commit-config-brancher-pr.sh(1 hunks).claude/.claude-plugin/skills/branching-day/commit-image-mirroring-pr.sh(1 hunks).claude/.claude-plugin/skills/branching-day/commit-tide-config-pr.sh(1 hunks).claude/.claude-plugin/skills/branching-day/run-config-brancher.sh(1 hunks).claude/.claude-plugin/skills/branching-day/run-tide-config-manager.sh(1 hunks).claude/.claude-plugin/skills/branching-day/update-etcd-config.sh(1 hunks).claude/.claude-plugin/skills/branching-day/update-image-mirroring.sh(1 hunks).claude/.claude-plugin/skills/branching-day/update-infra-periodics.sh(1 hunks).claude/.claude-plugin/skills/branching-day/update-tide-and-infra-jobs.sh(1 hunks).claude/.claude-plugin/skills/branching-day/verify-config-brancher.sh(1 hunks).claude/.claude-plugin/skills/branching-day/verify-etcd-config.sh(1 hunks).claude/.claude-plugin/skills/branching-day/verify-infra-periodics.sh(1 hunks).claude/.claude-plugin/skills/branching-day/verify-tide-and-infra-jobs.sh(1 hunks).claude/.claude-plugin/skills/branching-day/verify-tide-config-manager.sh(1 hunks).claude/.claude-plugin/skills/common/build-branching-tooling.sh(1 hunks).claude/.claude-plugin/skills/common/create-branch-from-master.sh(1 hunks).claude/.claude-plugin/skills/common/get-current-release.sh(1 hunks).claude/.claude-plugin/skills/common/trigger-periodic-jobs.sh(1 hunks).claude/.claude-plugin/skills/post-branching/commit-auto-config-brancher-pr.sh(1 hunks).claude/.claude-plugin/skills/post-branching/commit-fast-forward-pr.sh(1 hunks).claude/.claude-plugin/skills/post-branching/commit-image-mirroring-x-y-plus-2-pr.sh(1 hunks).claude/.claude-plugin/skills/post-branching/commit-jira-validation-pr.sh(1 hunks).claude/.claude-plugin/skills/post-branching/commit-merge-blockers-pr.sh(1 hunks).claude/.claude-plugin/skills/post-branching/create-config-jobs-x-y-plus-2.sh(1 hunks).claude/.claude-plugin/skills/post-branching/mirror-imagestreams-to-quay.sh(1 hunks).claude/.claude-plugin/skills/post-branching/tag-imagestreams.sh(1 hunks).claude/.claude-plugin/skills/post-branching/update-auto-config-brancher-x-y-plus-2.sh(1 hunks).claude/.claude-plugin/skills/post-branching/update-fast-forward-job.sh(1 hunks).claude/.claude-plugin/skills/post-branching/update-image-mirroring-x-y-plus-2.sh(1 hunks).claude/.claude-plugin/skills/post-branching/update-jira-validation-post-branching.sh(1 hunks).claude/.claude-plugin/skills/post-branching/update-merge-blockers-job.sh(1 hunks).claude/.claude-plugin/skills/post-branching/verify-auto-config-brancher.sh(1 hunks).claude/.claude-plugin/skills/post-branching/verify-config-jobs-x-y-plus-2.sh(1 hunks).claude/.claude-plugin/skills/post-branching/verify-fast-forward-job.sh(1 hunks).claude/.claude-plugin/skills/post-branching/verify-image-mirroring-x-y-plus-2.sh(1 hunks).claude/.claude-plugin/skills/post-branching/verify-imagestream-tags.sh(1 hunks).claude/.claude-plugin/skills/post-branching/verify-jira-validation.sh(1 hunks).claude/.claude-plugin/skills/post-branching/verify-merge-blockers-job.sh(1 hunks).claude/commands/branching-day.md(1 hunks).claude/commands/post-branching.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.claude/commands/branching-day.md
[grammar] ~186-~186: Use a hyphen to join words.
Context: ... - Report success when both complete (20 minute timeout) - Fail if either job fai...
(QB_NEW_EN_HYPHEN)
.claude/commands/post-branching.md
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...strap release X.Y+2 (current + 0.02), X.Y+3 (current + 0.03) - Use TodoWrite:...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Wrong calculation of X.Y+1, X.Y+2, or X.Y+3 - Action: Verify version arithmet...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
.claude/commands/branching-day.md
53-53: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
58-58: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
63-63: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
119-119: Spaces inside code span elements
(MD038, no-space-in-code)
221-221: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
.claude/.claude-plugin/skills/branching-day/verify-infra-periodics.sh
[warning] 13-13: CURRENT_RELEASE appears unused. Verify use (or export if used externally).
(SC2034)
.claude/.claude-plugin/skills/post-branching/verify-config-jobs-x-y-plus-2.sh
[warning] 13-13: XY_PLUS_1 appears unused. Verify use (or export if used externally).
(SC2034)
.claude/.claude-plugin/skills/post-branching/verify-image-mirroring-x-y-plus-2.sh
[warning] 13-13: XY_PLUS_1 appears unused. Verify use (or export if used externally).
(SC2034)
.claude/.claude-plugin/skills/post-branching/verify-jira-validation.sh
[warning] 13-13: CURRENT_RELEASE appears unused. Verify use (or export if used externally).
(SC2034)
.claude/.claude-plugin/skills/branching-day/verify-tide-and-infra-jobs.sh
[warning] 13-13: CURRENT_RELEASE appears unused. Verify use (or export if used externally).
(SC2034)
.claude/.claude-plugin/skills/common/trigger-periodic-jobs.sh
[warning] 36-36: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 69-69: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (35)
.claude/.claude-plugin/skills/branching-day/verify-tide-and-infra-jobs.sh (1)
28-35: LGTM with a note on grep context window.The verification correctly checks that
--current-releasewas updated. The-A 10context window assumes the flag appears within 10 lines of the job name, which is reasonable for typical YAML structure but could be fragile if the configuration grows significantly..claude/.claude-plugin/skills/post-branching/verify-config-jobs-x-y-plus-2.sh (4)
25-32: LGTM with a note on git assumptions.The commit count verification is reasonable. It assumes
origin/masterexists and is up-to-date, which should be true in the workflow context. The minimum threshold of 4 commits provides flexibility for variations in the automation process.
34-41: LGTM!The config files verification correctly uses
findto recursively search for release-specific configuration files and validates that at least one was created.
43-50: LGTM!The jobs verification appropriately checks for job files matching the target version.
52-65: LGTM!The optional verification checks for openshift-priv configs and template deprecation allowlist appropriately emit warnings rather than failures, allowing the workflow to continue even if these changes aren't present.
.claude/.claude-plugin/skills/branching-day/commit-tide-config-pr.sh (2)
4-7: LGTM!The argument validation and branch check provide good guardrails to ensure the script operates in the correct context and on the correct branch.
9-11: Clarify multiple commits of the same file.Lines 10-11 commit
infra-periodics.yamltwice with different messages. This pattern suggests the file should be modified between commits, but the script doesn't show any modification commands. This could work if:
- The file is modified externally between these commits
- Each commit captures staged changes from different prior update scripts
However, if all changes are already staged, the second commit will fail with "nothing to commit." Consider adding comments to clarify the expected state or add explicit checks.
🔎 Proposed clarification
+# Note: Assumes infra-periodics.yaml was modified for merge-blockers git add core-services/prow/ && git commit -m "tide-config-manager make prow-config" + +# Commit merge-blockers changes git add ci-operator/jobs/infra-periodics.yaml && git commit -m "bump periodic-openshift-release-merge-blockers" + +# Commit ocp-build-data-enforcer changes (assumes additional modifications) git add ci-operator/jobs/infra-periodics.yaml && git commit -m "bump periodic-ocp-build-data-enforcer".claude/.claude-plugin/skills/post-branching/update-jira-validation-post-branching.sh (1)
11-12: LGTM!The idempotency check appropriately prevents duplicate entries and exits gracefully if the configuration already exists.
.claude/.claude-plugin/skills/common/trigger-periodic-jobs.sh (3)
10-29: LGTM!The argument parsing correctly handles the optional
--pollingflag and validates that at least one job name is provided.
31-32: LGTM!Token retrieval correctly handles authentication failure with a clear error message and appropriate exit.
92-113: LGTM!The main loop correctly handles job triggering, tracks failures, and provides appropriate feedback based on whether polling is enabled. The exit code accurately reflects the overall operation status.
.claude/.claude-plugin/skills/common/get-current-release.sh (2)
4-7: LGTM!The script correctly handles an optional repository path with a sensible default and validates the presence of the required configuration file.
19-30: LGTM!The validation logic appropriately enforces the branching readiness requirement and provides clear, actionable error messages when the criteria aren't met. The success path correctly outputs the version for downstream use.
.claude/.claude-plugin/skills/post-branching/commit-fast-forward-pr.sh (2)
4-7: LGTM!The argument validation and branch check provide appropriate safeguards to ensure the script operates in the correct context.
9-14: LGTM!The conditional commit logic correctly stages the file and only creates a commit if there are actual changes to commit. The descriptive multi-line commit message provides good context for the change.
.claude/.claude-plugin/skills/post-branching/update-merge-blockers-job.sh (1)
12-14: Verify sed range pattern edge cases.The range pattern
/name: periodic-openshift-release-merge-blockers$/,/name:/assumes there's anothername:field after the target block. Ifperiodic-openshift-release-merge-blockersis the last job in the file, if the job ordering changes, or if there are multiple jobs with "merge-blockers" in their name, the substitution might behave unexpectedly. Verify the actual YAML structure and consider using a more explicit range delimiter or a line-specific substitution if reliability is a concern..claude/.claude-plugin/skills/branching-day/verify-config-brancher.sh (1)
22-37: LGTM! Verification logic is correct.The script correctly uses
git diff --quietto detect changes and handles thegrep -cwith|| trueto preventpipefailfrom aborting when no matches are found..claude/.claude-plugin/skills/post-branching/commit-auto-config-brancher-pr.sh (1)
1-15: LGTM! Commit script follows consistent pattern.The script correctly validates arguments, checks the branch name, and stages/commits changes with appropriate error handling via command chaining.
.claude/.claude-plugin/skills/post-branching/verify-auto-config-brancher.sh (1)
26-27:grep -qin pipeline fails withpipefailwhen pattern not found.Line 26 pipes output to
grep -q, which exits non-zero when the pattern--future-release=${XY_PLUS_2}is not found. Becauseset -euo pipefailis active (line 5), the entire script terminates immediately without settingVERIFICATION_FAILED=1or printing the intended error message.🔎 Proposed fix
-if grep -A 10 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \ - grep -q -- "--future-release=${XY_PLUS_2}"; then +if grep -A 10 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \ + grep -- "--future-release=${XY_PLUS_2}" >/dev/null 2>&1; then echo "✓ --future-release updated to ${XY_PLUS_2}" else echo "✗ FAILED: --future-release not updated correctly" VERIFICATION_FAILED=1 fiLikely an incorrect or invalid review comment.
.claude/.claude-plugin/skills/common/build-branching-tooling.sh (1)
1-11: LGTM!The script initialization, argument handling, and validation logic are well-structured. The use of strict error handling (
set -euo pipefail) and Makefile validation ensures safe execution..claude/.claude-plugin/skills/post-branching/mirror-imagestreams-to-quay.sh (2)
1-24: LGTM!The script initialization, argument validation, and stream configuration are well-organized. The use of an array for stream definitions makes the code maintainable and clear.
40-57: LGTM!The main mirroring loop correctly handles multiple streams, aggregates failures, and provides clear status reporting. The use of parameter expansion for namespace/name extraction is idiomatic.
.claude/.claude-plugin/skills/post-branching/commit-jira-validation-pr.sh (1)
1-15: LGTM!The script correctly validates arguments and branch context, uses guarded git operations to avoid unnecessary commits, and provides a clear, multi-line commit message. The staged-changes check (
git diff --staged --quiet) ensures commits are only made when there are actual changes..claude/.claude-plugin/skills/branching-day/verify-infra-periodics.sh (2)
26-33: LGTM!The verification correctly checks both the
base_refand--current-releaseflag for the future release in the infra-periodics configuration.
53-61: LGTM!The final verification summary and exit logic correctly aggregates all verification results and provides clear status reporting.
.claude/.claude-plugin/skills/branching-day/commit-config-brancher-pr.sh (3)
1-10: LGTM!The script initialization and pre-commit reset logic ensure a clean starting state. The branch validation prevents accidental commits to the wrong branch.
11-19: LGTM!The staged commit workflow correctly handles the config-brancher changes separately from generated updates. The error suppression on Lines 16-17 (
2>/dev/null || true) is appropriate since the files being reset may not exist in all repository configurations.
21-31: LGTM!The conditional commit logic for optional files (infra-periodics and Jira config) correctly checks for file existence before attempting to stage and commit. The final commit count provides useful feedback.
.claude/.claude-plugin/skills/common/create-branch-from-master.sh (2)
16-22: LGTM!The master update logic correctly handles both reset and rebase scenarios with appropriate error handling. The ability to choose between destructive reset and safe rebase provides valuable flexibility for different use cases.
24-27: LGTM!The branch creation logic safely handles existing branches and provides clear feedback. The deletion check using
git show-refis more robust than checking exit codes fromgit branch -D..claude/.claude-plugin/skills/post-branching/update-image-mirroring-x-y-plus-2.sh (1)
1-16: LGTM!The script initialization and idempotency check (Line 16) are well-designed. Skipping already-present keys prevents duplicate entries and makes the script safe to re-run.
.claude/.claude-plugin/skills/post-branching/tag-imagestreams.sh (1)
15-21: LGTM! Fail-fast approach with clear operations.The script correctly uses
set -euo pipefailto ensure failures in the pipeline halt execution. The operations are clear and the system:admin escalation is appropriate for imagestream management..claude/.claude-plugin/plugin.json (1)
1-225: LGTM! Well-structured plugin manifest.The JSON structure is clean and well-organized. The skill definitions are comprehensive with clear descriptions, and the separation between branching-day and post-branching workflows is logical.
.claude/.claude-plugin/skills/post-branching/create-config-jobs-x-y-plus-2.sh (2)
1-5: LGTM!The strict error handling with
set -euo pipefailand argument validation are appropriate for this automation script.
56-61: LGTM!The cleanup, final job generation, and summary output are appropriate for completing the workflow.
| set -euo pipefail | ||
|
|
||
| [ "$#" -ne 3 ] && echo "Usage: $0 <current-release> <future-release> <release-repo>" && exit 1 | ||
| [ ! -d "$2" ] && echo "Error: Release repo not found" && exit 1 |
There was a problem hiding this comment.
Fix incorrect directory validation.
The script checks if $2 (future-release version string) is a directory, but it should check $3 (release-repo path). This causes the script to always fail with a misleading error message.
🔎 Proposed fix
-[ ! -d "$2" ] && echo "Error: Release repo not found" && exit 1
+[ ! -d "$3" ] && echo "Error: Release repo not found" && exit 1📝 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.
| [ ! -d "$2" ] && echo "Error: Release repo not found" && exit 1 | |
| [ ! -d "$3" ] && echo "Error: Release repo not found" && exit 1 |
🤖 Prompt for AI Agents
In .claude/.claude-plugin/skills/branching-day/run-config-brancher.sh around
line 5, the script erroneously checks whether $2 (the future-release version
string) is a directory, causing a false "Release repo not found" error; change
the test to check $3 (the release-repo path) instead and keep the same
error/exit behavior so the script validates the repository path correctly.
|
|
||
| [ ! -f "$ETCD" ] && echo "Error: etcd config not found" && exit 1 | ||
|
|
||
| sed -i "/- openshift-4.20$/a\ - openshift-${CURRENT}" "$ETCD" |
There was a problem hiding this comment.
Critical: Hardcoded version "openshift-4.20" will cause failures.
The script hardcodes "openshift-4.20" as an anchor point for inserting the current release. This creates multiple problems:
- Breaks when OpenShift moves beyond 4.20: Once 4.20 is replaced with a newer version in the config, this script will fail to find the anchor.
- Maintenance burden: Every time the anchor version changes, all automation needs to be updated.
- Silent failure: If "openshift-4.20" isn't found,
sedsilently does nothing.
🔎 Dynamic anchor detection
-sed -i "/- openshift-4.20$/a\ - openshift-${CURRENT}" "$ETCD"
+# Find the last openshift version before our current release
+ANCHOR=$(grep -E "^\s+- openshift-[0-9]+\.[0-9]+$" "$ETCD" | \
+ grep -v "openshift-${CURRENT}" | \
+ tail -1 | \
+ sed 's/.*\(openshift-[0-9.]*\).*/\1/')
+
+if [ -z "$ANCHOR" ]; then
+ echo "Error: Could not find anchor version in etcd config"
+ exit 1
+fi
+
+sed -i "/- ${ANCHOR}$/a\ - openshift-${CURRENT}" "$ETCD"This dynamically finds the last version entry before the current release to use as an anchor.
📝 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.
| sed -i "/- openshift-4.20$/a\ - openshift-${CURRENT}" "$ETCD" | |
| # Find the last openshift version before our current release | |
| ANCHOR=$(grep -E "^\s+- openshift-[0-9]+\.[0-9]+$" "$ETCD" | \ | |
| grep -v "openshift-${CURRENT}" | \ | |
| tail -1 | \ | |
| sed 's/.*\(openshift-[0-9.]*\).*/\1/') | |
| if [ -z "$ANCHOR" ]; then | |
| echo "Error: Could not find anchor version in etcd config" | |
| exit 1 | |
| fi | |
| sed -i "/- ${ANCHOR}$/a\ - openshift-${CURRENT}" "$ETCD" |
| [ "$#" -ne 2 ] && echo "Usage: $0 <x.y+2> <release-repo>" && exit 1 | ||
|
|
||
| cd "$2" | ||
| [ "$(git branch --show-current)" != "image-mirror-$1" ] && echo "Error: Wrong branch" && exit 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all references to image-mirror and image-mirroring patterns
rg -n 'image-mirror(ing)?-' .claude/ --type shRepository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Also search for the create-branch-from-master script to understand the convention
fd -t f 'create-branch' .claude/Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Check the post-branching directory for related scripts
find .claude/.claude-plugin/skills/post-branching -type f -name "*.sh" | head -20Repository: openshift/ci-tools
Length of output: 1588
🏁 Script executed:
# Read the commit-image-mirroring script
cat -n .claude/.claude-plugin/skills/post-branching/commit-image-mirroring-x-y-plus-2-pr.shRepository: openshift/ci-tools
Length of output: 573
🏁 Script executed:
# Check the update script for consistency
cat -n .claude/.claude-plugin/skills/post-branching/update-image-mirroring-x-y-plus-2.shRepository: openshift/ci-tools
Length of output: 1353
🏁 Script executed:
# Check the verify script for consistency
cat -n .claude/.claude-plugin/skills/post-branching/verify-image-mirroring-x-y-plus-2.shRepository: openshift/ci-tools
Length of output: 2498
🏁 Script executed:
# Search for branch creation patterns
rg -n 'checkout.*-b.*image' .claude/ --type shRepository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Search for references to the branch name in all scripts
rg -n '"image-mirror' .claude/ --type shRepository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Check if there's a post-branching.md or similar documentation
find .claude -name "*.md" -type f | xargs grep -l "image-mirror" 2>/dev/nullRepository: openshift/ci-tools
Length of output: 95
🏁 Script executed:
# Read the branching-day.md file to understand the branch naming convention
cat .claude/commands/branching-day.mdRepository: openshift/ci-tools
Length of output: 14071
Fix branch name check from image-mirror-$1 to image-mirroring-$1.
Line 7 checks for branch image-mirror-$1, but the documented branching process (branching-day.md, Step 7) creates the branch as image-mirroring-<future-release>. This mismatch will cause the script to fail with "Error: Wrong branch" even when following the correct procedure. The script filename and all related scripts (update-image-mirroring, verify-image-mirroring) also use the "mirroring" convention.
🤖 Prompt for AI Agents
.claude/.claude-plugin/skills/post-branching/commit-image-mirroring-x-y-plus-2-pr.sh
around line 7: the branch-name check currently expects "image-mirror-$1" but the
repo uses the "image-mirroring-$1" convention; update the conditional to compare
against "image-mirroring-$1" (i.e., replace "image-mirror-$1" with
"image-mirroring-$1") so the script accepts branches created per
branching-day.md and the other related scripts.
| set -euo pipefail | ||
|
|
||
| [ "$#" -ne 3 ] && echo "Usage: $0 <current> <x.y+2> <release-repo>" && exit 1 | ||
|
|
There was a problem hiding this comment.
Fix incorrect directory validation.
The script checks if $2 (x.y+2 version string) is a directory, but it should check $3 (release-repo path). This causes the script to always fail with a misleading error message.
🔎 Proposed fix
-[ ! -d "$2" ] && echo "Error: Release repo not found" && exit 1
+[ ! -d "$3" ] && echo "Error: Release repo not found" && exit 1Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .claude/.claude-plugin/skills/post-branching/update-fast-forward-job.sh
around line 5 the script incorrectly validates the release-repo path by testing
if "$2" is a directory; change the test to check "$3" instead and update the
error message to reference the release-repo path variable so the correct
parameter is validated and the error text is not misleading.
| awk -v new_stanza="$OPENSHIFT_STANZA" -v prev="openshift-${XY_PLUS_1}:" ' | ||
| { | ||
| if ($0 ~ prev) { in_stanza = 1 } | ||
| else if (in_stanza && /^ [a-z]/ && !/^ /) { print new_stanza; in_stanza = 0 } | ||
| }' "$JIRA" > "${JIRA}.tmp" | ||
|
|
||
| awk -v new_stanza="$RELEASE_STANZA" -v prev="release-${XY_PLUS_1}:" ' | ||
| { | ||
| if ($0 ~ prev) { in_stanza = 1 } | ||
| else if (in_stanza && (/^ [a-z]/ && !/^ / || /^[a-z]/)) { print new_stanza; in_stanza = 0 } | ||
| }' "${JIRA}.tmp" > "${JIRA}.tmp2" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure to locate the config file
find . -type f -name "*.yaml" -o -name "*.yml" | grep -E "(jira|config)" | head -20Repository: openshift/ci-tools
Length of output: 2376
🏁 Script executed:
# Check the directory structure mentioned in the review
ls -la ".claude/.claude-plugin/skills/post-branching/" 2>/dev/null || echo "Directory not found at that path"Repository: openshift/ci-tools
Length of output: 1780
🏁 Script executed:
# Look for any jira-related config files in the repository
fd -t f -e yaml -e yml | grep -i jiraRepository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
cat -n ".claude/.claude-plugin/skills/post-branching/update-jira-validation-post-branching.sh"Repository: openshift/ci-tools
Length of output: 2024
🏁 Script executed:
# Find the actual jira-lifecycle-plugin config file
fd -t f "config.yaml" | grep "jira-lifecycle-plugin"Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Look for the core-services directory structure
find . -type d -name "core-services" 2>/dev/nullRepository: openshift/ci-tools
Length of output: 498
🏁 Script executed:
# Search for any jira-lifecycle-plugin files in the test directories
find ./test/integration -name "*jira*" -type fRepository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Check if config files exist in the core-services test dirs
for dir in ./test/integration/*/expected/core-services ./test/integration/*/input/core-services; do
if [ -d "$dir" ]; then
echo "=== $dir ==="
ls -la "$dir" 2>/dev/null | head -20
fi
doneRepository: openshift/ci-tools
Length of output: 628
🏁 Script executed:
# Look for tests related to jira validation post-branching
rg -l "update-jira-validation-post-branching|jira.*stanza" --type shRepository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Check the verify-jira-validation.sh script which likely has test fixtures
cat -n ".claude/.claude-plugin/skills/post-branching/verify-jira-validation.sh"Repository: openshift/ci-tools
Length of output: 2959
🏁 Script executed:
# Search for any YAML files that might show the jira config structure
find . -type f -name "*.yaml" -o -name "*.yml" | xargs grep -l "dependent_bug_states\|validate_by_default" 2>/dev/null | head -10Repository: openshift/ci-tools
Length of output: 878
🏁 Script executed:
# Look for any test fixtures or examples with similar stanza patterns
rg -A 15 "openshift-.*:" --type yaml | grep -A 15 "dependent_bug" | head -50Repository: openshift/ci-tools
Length of output: 6604
🏁 Script executed:
# Look at one of the fixture files to understand the full structure
cat -n "./cmd/branchingconfigmanagers/bugzilla-config-manager/testdata/zz_fixture_TestRun_lifecycle_open_and_codefree_e.yaml.yaml" | head -100Repository: openshift/ci-tools
Length of output: 1279
🏁 Script executed:
# Also look at the jira lifecycle input files to understand the structure better
find ./cmd/branchingconfigmanagers -name "lifecycle_*.yaml" -type f | head -1 | xargs cat -n | head -80Repository: openshift/ci-tools
Length of output: 946
🏁 Script executed:
# Examine one of the actual lifecycle test input files to see the full structure
cat "./cmd/branchingconfigmanagers/bugzilla-config-manager/testdata/lifecycle_open_and_codefreeze.yaml"Repository: openshift/ci-tools
Length of output: 194
🏁 Script executed:
# Let's also check if there are any tests for the update-jira-validation script
find . -type f -name "*.sh" -o -name "*.go" | xargs grep -l "update-jira-validation" 2>/dev/nullRepository: openshift/ci-tools
Length of output: 46
🏁 Script executed:
# Let's trace through the awk logic with a test case to verify the behavior
# Create a test YAML file similar to the jira config structure
cat > /tmp/test_jira.yaml << 'EOF'
default:
main:
target_version: 4.9.0
openshift-4.8:
dependent_bug_states:
- status: MODIFIED
target_version: 4.8.0
validate_by_default: true
openshift-4.9:
dependent_bug_states:
- status: MODIFIED
target_version: 4.9.0
validate_by_default: true
release-4.8:
dependent_bug_states:
- status: MODIFIED
target_version: 4.8.0
validate_by_default: true
release-4.9:
dependent_bug_states:
- status: MODIFIED
target_version: 4.9.0
validate_by_default: true
EOF
# Now test the awk pattern that inserts after openshift-4.8
NEW_STANZA=" openshift-4.10:
dependent_bug_states:
- status: MODIFIED
target_version: 4.10.0"
echo "=== Testing first awk pattern (openshift insertion) ==="
awk -v new_stanza="$NEW_STANZA" -v prev="openshift-4.8:" '
{
if ($0 ~ prev) { in_stanza = 1 }
else if (in_stanza && /^ [a-z]/ && !/^ /) { print new_stanza; in_stanza = 0 }
print
}' /tmp/test_jira.yaml
echo ""
echo "=== Testing edge case: stanza as last entry ==="
cat > /tmp/test_jira_last.yaml << 'EOF'
default:
openshift-4.8:
target_version: 4.8.0
EOF
awk -v new_stanza="$NEW_STANZA" -v prev="openshift-4.8:" '
{
if ($0 ~ prev) { in_stanza = 1 }
else if (in_stanza && /^ [a-z]/ && !/^ /) { print new_stanza; in_stanza = 0 }
print
}' /tmp/test_jira_last.yaml
echo ""
echo "=== Second awk pattern test (release insertion) ==="
RELEASE_STANZA=" release-4.10:
dependent_bug_states:
- status: MODIFIED
target_version: 4.10.0"
awk -v new_stanza="$RELEASE_STANZA" -v prev="release-4.8:" '
{
if ($0 ~ prev) { in_stanza = 1 }
else if (in_stanza && (/^ [a-z]/ && !/^ / || /^[a-z]/)) { print new_stanza; in_stanza = 0 }
print
}' /tmp/test_jira.yamlRepository: openshift/ci-tools
Length of output: 1511
🏁 Script executed:
# Test the critical edge case: when the stanza being replaced is the LAST one
echo "=== Critical edge case: Last stanza in file ==="
cat > /tmp/test_jira_last_release.yaml << 'EOF'
default:
openshift-4.8:
dependent_bug_states:
- status: MODIFIED
target_version: 4.8.0
release-4.8:
dependent_bug_states:
- status: MODIFIED
target_version: 4.8.0
validate_by_default: true
EOF
RELEASE_STANZA=" release-4.10:
dependent_bug_states:
- status: MODIFIED
target_version: 4.10.0"
echo "Input file:"
cat /tmp/test_jira_last_release.yaml
echo ""
echo "Output after awk (should insert release-4.10 between release-4.8 and EOF):"
awk -v new_stanza="$RELEASE_STANZA" -v prev="release-4.8:" '
{
if ($0 ~ prev) { in_stanza = 1 }
else if (in_stanza && (/^ [a-z]/ && !/^ / || /^[a-z]/)) { print new_stanza; in_stanza = 0 }
print
}' /tmp/test_jira_last_release.yaml
echo ""
echo "=== Checking END OF FILE handling in first awk pattern ==="
cat > /tmp/test_openshift_last.yaml << 'EOF'
default:
openshift-4.8:
dependent_bug_states:
- status: MODIFIED
target_version: 4.8.0
EOF
OPENSHIFT_STANZA=" openshift-4.10:
dependent_bug_states:
- status: MODIFIED
target_version: 4.10.0"
echo "Input file (openshift-4.8 is last):"
cat /tmp/test_openshift_last.yaml
echo ""
echo "Output after awk (should insert openshift-4.10):"
awk -v new_stanza="$OPENSHIFT_STANZA" -v prev="openshift-4.8:" '
{
if ($0 ~ prev) { in_stanza = 1 }
else if (in_stanza && /^ [a-z]/ && !/^ /) { print new_stanza; in_stanza = 0 }
print
}' /tmp/test_openshift_last.yaml
echo ""
echo "=== Fixed version with END block ==="
awk -v new_stanza="$OPENSHIFT_STANZA" -v prev="openshift-4.8:" '
{
if ($0 ~ prev) { in_stanza = 1 }
else if (in_stanza && /^ [a-z]/ && !/^ /) { print new_stanza; in_stanza = 0 }
print
}
END {
if (in_stanza) { print new_stanza }
}' /tmp/test_openshift_last.yamlRepository: openshift/ci-tools
Length of output: 1220
Fix end-of-file handling in both awk patterns.
Both awk commands fail to insert new stanzas when the target stanza is the last entry in the file. The state machine flag in_stanza remains true but never prints the new stanza because no subsequent line matches the exit condition. Add an END block to each awk command to handle this case:
END {
if (in_stanza) print new_stanza
}Apply this to both the openshift (lines 34-39) and release (lines 41-46) stanza insertions. The second awk pattern's exit condition is slightly more robust (includes /^[a-z]/ for top-level keys), but both have the same EOF bug.
🤖 Prompt for AI Agents
.claude/.claude-plugin/skills/post-branching/update-jira-validation-post-branching.sh
around lines 34 to 46: both awk invocations set an in_stanza flag when they find
the target stanza but never handle the case where that stanza is the final entry
in the file, so the new stanza isn't emitted at EOF; add an END block to each
awk script that checks if in_stanza is still true and prints new_stanza (i.e.,
implement END { if (in_stanza) print new_stanza }) so the new stanza is appended
when the matched stanza is last in the file.
| if grep -A 10 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \ | ||
| grep -q -- "--future-release=${XY_PLUS_1}"; then |
There was a problem hiding this comment.
Same pipefail issue on second verification check.
Line 35 has the identical problem: grep -q in a pipeline with pipefail causes premature script termination when the old value is correctly absent.
🔎 Proposed fix
-if grep -A 10 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \
- grep -q -- "--future-release=${XY_PLUS_1}"; then
+if grep -A 10 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \
+ grep -- "--future-release=${XY_PLUS_1}" >/dev/null 2>&1; then
echo "✗ FAILED: Old value ${XY_PLUS_1} still present"
VERIFICATION_FAILED=1
else
echo "✓ Old value ${XY_PLUS_1} removed"
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.
| if grep -A 10 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \ | |
| grep -q -- "--future-release=${XY_PLUS_1}"; then | |
| if grep -A 10 "name: periodic-prow-auto-config-brancher" "$INFRA_PERIODICS" | \ | |
| grep -- "--future-release=${XY_PLUS_1}" >/dev/null 2>&1; then | |
| echo "✗ FAILED: Old value ${XY_PLUS_1} still present" | |
| VERIFICATION_FAILED=1 | |
| else | |
| echo "✓ Old value ${XY_PLUS_1} removed" | |
| fi |
🤖 Prompt for AI Agents
In .claude/.claude-plugin/skills/post-branching/verify-auto-config-brancher.sh
around lines 35-36, the pipeline using grep -A ... | grep -q is triggering a
fatal exit under set -o pipefail when the first grep doesn't find the block;
change the pipeline so its non-zero exit won’t abort the script — for example
append "|| true" to the end of the pipeline (or otherwise swallow the pipeline
exit) and then check the second grep's result explicitly, ensuring the script
does not prematurely terminate when the old value is absent.
|
Scheduling required tests: Scheduling tests matching the |
|
@Prucek: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Add comprehensive Claude Code plugins to automate OpenShift release branching workflows.
Features
/branching-dayand/post-branchingWorkflow Coverage
Branching Day: 10 steps → 3 PRs (config-brancher, tide-config, image-mirroring)
Post-Branching: 10 steps → 5 PRs (jira, merge-blockers, fast-forward, ci-configs, auto-config-brancher)
🤖 Generated with Claude Code