Using a shell function to mirror instead of repeating the code#5249
Open
hector-vido wants to merge 1 commit into
Open
Using a shell function to mirror instead of repeating the code#5249hector-vido wants to merge 1 commit into
hector-vido wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughThis PR consolidates image mirroring and tag import retry logic by extracting hardcoded shell loops into parameterized ChangesPromotion image mirroring and tagging refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 15 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hector-vido 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/steps/release/promote_test.go`:
- Around line 1116-1119: The test currently only checks that the output contains
a substring (variable got vs shouldBe) which allows retry/fail behavior
regressions to pass; update the assertion to validate the full, exact generated
command and explicitly assert the retry semantics tokens (e.g., the "mirror 5 2
..." retry/count parameters) and any retry/fail markers produced by the helper
by replacing the contains check with an equality check against the fully
expected command string and/or additional assertions that inspect retry counts
or helper call outcomes (refer to the variables got and shouldBe and the "mirror
5 2 /etc/push-secret/.dockerconfigjson src=dst" token to locate the relevant
assertion to change).
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml`:
- Around line 37-41: The retry loop around the oc tag command (the block using
"set +e" / "set -e" and "for r in {1..2}; do ... && break; :; done") currently
masks failures because the trailing ":" always returns success; update the loop
to detect exhaustion and fail explicitly: e.g., introduce a success flag
(tag_success) set when oc tag succeeds and after the loop check that flag and
exit non‑zero if unset, or replace the no-op ":" fallback with an explicit
non-zero exit when all attempts fail so the script does not continue silently;
modify the loop and add a post-loop failure check around the oc tag retry logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0a1e7962-e5fe-4010-921e-2f5808cbf587
📒 Files selected for processing (9)
pkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__arm64_only.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__multi_architecture.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
hector-vido
force-pushed
the
promotion-shell-squash
branch
from
5245b4c to
90b17da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__multi_architecture.yaml`:
- Around line 11-22: The generated shell `mirror` function uses brace expansion
`for r in {1..$1}` which fails when $1 is dynamic; update the generator in
mirrorShellFunction (in pkg/steps/release/promote.go) to emit a portable loop
such as `for r in $(seq 1 $1)` or a C-style loop `for ((r=1; r<=$1; r++))`
instead of the brace form so the produced fixtures (mirror function in the
YAMLs) iterate correctly; modify the string/template returned by
mirrorShellFunction accordingly and run tests to regenerate the affected
zz_fixture_* YAMLs.
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml`:
- Around line 29-33: The retry stops too early because getTagLoopCommand's
generated tagCmd is the shell loop "while read tag; do oc tag ...; done <<EOF
..." whose exit code is only the last oc invocation; change getTagLoopCommand so
the loop accumulates failures (e.g., set a local status variable, run each oc
tag and if it fails set status=1) and after the loop run "exit $status" so
tagCmd returns non-zero if any oc tag failed; keep retryLoopTemplate using
"tagCmd && break" so retries only stop when the entire batch of tags succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 86c86060-88b1-4845-a30d-86f4ed3e7756
📒 Files selected for processing (9)
pkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__arm64_only.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__multi_architecture.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/steps/release/promote_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__arm64_only.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
- pkg/steps/release/promote.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
Comment on lines
+11
to
+22
| function mirror { | ||
| for r in {1..$1}; do | ||
| echo Mirror attempt $r | ||
| if oc image mirror --loglevel=2 --keep-manifest-list --registry-config=/etc/push-secret/.dockerconfigjson --max-per-registry=10 docker-registry.default.svc:5000/ci-op-y2n8rsh3/pipeline@sha256:afd71aa3cbbf7d2e00cd8696747b2abf164700147723c657919c20b13d13ec62=registry.ci.openshift.org/ci/applyconfig:latest docker-registry.default.svc:5000/ci-op-y2n8rsh3/pipeline@sha256:bbb=registry.ci.openshift.org/ci/bin:latest; then break; fi | ||
| if [ "${r}" -eq 5 ]; then | ||
| if oc image mirror --loglevel=$2 --keep-manifest-list --registry-config=$3 --max-per-registry=10 $4; then break; fi | ||
| if [ "${r}" -eq $1 ]; then | ||
| exit 1 | ||
| fi | ||
| backoff=$(($RANDOM % 120))s | ||
| echo Sleeping randomized $backoff before retry | ||
| sleep $backoff | ||
| done | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Brace expansion bug in mirrorShellFunction (affects zz_fixture_TestGetPromotionPod_basic_case__multi_architecture.yaml and zz_fixture_TestGetPromotionPod_promotion_quay.yaml).
Both fixtures contain the same broken for r in {1..$1} loop from the generated mirror function. The fix must be in pkg/steps/release/promote.go where mirrorShellFunction is defined. Replace with $(seq 1 $1) or a C-style for loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__multi_architecture.yaml`
around lines 11 - 22, The generated shell `mirror` function uses brace expansion
`for r in {1..$1}` which fails when $1 is dynamic; update the generator in
mirrorShellFunction (in pkg/steps/release/promote.go) to emit a portable loop
such as `for r in $(seq 1 $1)` or a C-style loop `for ((r=1; r<=$1; r++))`
instead of the brace form so the produced fixtures (mirror function in the
YAMLs) iterate correctly; modify the string/template returned by
mirrorShellFunction accordingly and run tests to regenerate the affected
zz_fixture_* YAMLs.
Comment on lines
+29
to
+33
| quay.io/openshift/ci:ci_c_latest=quay.io/openshift/ci:ci_c_latest__post1\nfi\nfor | ||
| r in {1..2}; do echo \"Tag attempt $r\"; \nwhile read tag; do\noc tag --source=docker | ||
| --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference | ||
| $tag\ndone <<EOF\nquay-proxy.ci.openshift.org/openshift/ci@sha256:ddd ci/${component}-quay:c\nquay-proxy.ci.openshift.org/openshift/ci@sha256:bbb | ||
| ci/ci-quay:${component}\nEOF && break; :; done" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the tag loop is generated in promote.go
rg -n -A10 'getTagLoopCommand|retryLoopTemplate' pkg/steps/release/promote.goRepository: openshift/ci-tools
Length of output: 1778
🏁 Script executed (no clone):
#!/bin/bash
set -u
run_case() {
local label="$1"
local inner_cmd="$2" # evaluated inside loop; can refer to $tag
echo "== $label =="
# tagCmd similar to getTagLoopCommand output
tagCmd=$(cat <<'EOF'
while read tag; do
__INNER__
done <<'DATA'
one
two
three
DATA
EOF
)
tagCmd="${tagCmd/__INNER__/$inner_cmd}"
# capture exit status
bash -c "$tagCmd"
echo "exit=$?"
echo
}
# 1) All succeed (true)
run_case "all succeed" "true"
# 2) fail for first tag, succeed for rest
run_case "fail first only" 'if [[ "$tag" == "one" ]]; then false; else true; fi'
# 3) fail for last tag
run_case "fail last only" 'if [[ "$tag" == "three" ]]; then false; else true; fi'
# 4) fail on any tag (always false)
run_case "always fail" "false"
Length of output: 142
Fix tag retry semantics: stop retrying only when all oc tag calls succeed.
retryLoopTemplate uses tagCmd && break, so whether retries stop depends on tagCmd’s exit status. getTagLoopCommand renders while read tag; do oc tag ...; done <<EOF ... EOF; in bash, the loop’s exit code is the exit status of the last oc tag run (EOF isn’t what makes it fail). That means if an earlier tag fails but later tags succeed, tagCmd exits 0 and the retry loop breaks—leaving the failed tags un-retried. Track failures across all tags (e.g., accumulate a non-zero status and exit $status) so any oc tag failure causes tagCmd to fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml`
around lines 29 - 33, The retry stops too early because getTagLoopCommand's
generated tagCmd is the shell loop "while read tag; do oc tag ...; done <<EOF
..." whose exit code is only the last oc invocation; change getTagLoopCommand so
the loop accumulates failures (e.g., set a local status variable, run each oc
tag and if it fails set status=1) and after the loop run "exit $status" so
tagCmd returns non-zero if any oc tag failed; keep retryLoopTemplate using
"tagCmd && break" so retries only stop when the entire batch of tags succeeds.
Contributor
|
@hector-vido: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The objective of these modifications is to reduce the total size of shell arguments and environment variables.
Promotion Step Shell Script Refactoring (summary)
This PR refactors how the promotion step builds the shell script run in the promotion Pod by introducing a reusable shell helper to perform image mirroring and by consolidating per-tag tagging logic into loop helpers. The change reduces repeated inline shell code, shrinking command/argument size and making retry/backoff behavior consistent across mirror operations.
What changed in practical terms
Behavioral and operational impact for CI users/operators
Tests and fixtures
Notes