Expand consolidated Quay promotion to OCP 4.11–4.22#5198
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@deepsm007: This pull request references DPTP-4340 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
📝 WalkthroughWalkthroughThis PR replaces single-version hardcoded checks with a version-range predicate (4.11–4.22, excluding 4.23 and 5.0) to determine Quay promotion eligibility. The new ChangesConsolidate Quay Promotion Eligibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/api/promotion.go (1)
110-112: ⚡ Quick winUpdate outdated format comment in
getQuayProxyTarget.Line 111 still documents a
-quaysuffix, but Lines 114/129/136 now generatenamespace/name:tagwithout it.Suggested doc fix
-// Format: namespace/imagestream-name-quay:tag +// Format: namespace/imagestream-name:tag🤖 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/api/promotion.go` around lines 110 - 112, The comment for getQuayProxyTarget is outdated (mentions a "-quay" suffix) — update the function docstring to reflect the actual format produced by getQuayProxyTarget (i.e., "namespace/imagestream-name:tag" or "namespace/name:tag") and remove the "-quay" mention; ensure the doc references the ImageStreamTagReference parameter and clearly shows the exact output format used by the function.pkg/steps/release/create_release_test.go (1)
99-112: ⚡ Quick winConsider table-driven testing for broader version coverage.
The
assemble_scripttest now only verifies version 4.15. IfbuildOcAdmReleaseNewCommandnow always uses the shell-based approach regardless of version (per the layer description), consider extending this to a table-driven test covering multiple versions (e.g., 4.10, 4.12, 4.15) to verify the shell script generation works correctly with version-dependent flags across all supported versions.♻️ Suggested table-driven structure
t.Run("assemble_script", func(t *testing.T) { - srcPol := imagev1.SourceTagReferencePolicy - config := &api.ReleaseTagConfiguration{Name: "4.15", ReferencePolicy: &srcPol} - got := buildOcAdmReleaseNewCommand(config, "test-ns", "stable", "cvo-pullspec", "dest:tag", "0.0.1-ver") - want := `_CI_RELEASE_IS_FILE="/tmp/ci-operator-release-is-stable.yaml" + srcPol := imagev1.SourceTagReferencePolicy + tests := []struct { + name string + config *api.ReleaseTagConfiguration + want string + }{ + { + name: "4.12 with ReferencePolicy", + config: &api.ReleaseTagConfiguration{Name: "4.12", ReferencePolicy: &srcPol}, + want: `_CI_RELEASE_IS_FILE="/tmp/ci-operator-release-is-stable.yaml" +if oc get imagestream "stable" -n "test-ns" -o yaml > "${_CI_RELEASE_IS_FILE}" 2>/dev/null; then + oc adm release new --max-per-registry=32 -n test-ns --from-image-stream-file ${_CI_RELEASE_IS_FILE} --to-image-base cvo-pullspec --to-image dest:tag --name 0.0.1-ver --keep-manifest-list || oc adm release new --max-per-registry=32 -n test-ns --from-image-stream stable --to-image-base cvo-pullspec --to-image dest:tag --name 0.0.1-ver --keep-manifest-list +else + oc adm release new --max-per-registry=32 -n test-ns --from-image-stream stable --to-image-base cvo-pullspec --to-image dest:tag --name 0.0.1-ver --keep-manifest-list +fi`, + }, + { + name: "4.15 with ReferencePolicy", + config: &api.ReleaseTagConfiguration{Name: "4.15", ReferencePolicy: &srcPol}, + want: `_CI_RELEASE_IS_FILE="/tmp/ci-operator-release-is-stable.yaml" if oc get imagestream "stable" -n "test-ns" -o yaml > "${_CI_RELEASE_IS_FILE}" 2>/dev/null; then oc adm release new --max-per-registry=32 -n test-ns --from-image-stream-file ${_CI_RELEASE_IS_FILE} --to-image-base cvo-pullspec --to-image dest:tag --name 0.0.1-ver --reference-mode=source --keep-manifest-list || oc adm release new --max-per-registry=32 -n test-ns --from-image-stream stable --to-image-base cvo-pullspec --to-image dest:tag --name 0.0.1-ver --reference-mode=source --keep-manifest-list else oc adm release new --max-per-registry=32 -n test-ns --from-image-stream stable --to-image-base cvo-pullspec --to-image dest:tag --name 0.0.1-ver --reference-mode=source --keep-manifest-list -fi` - if diff := cmp.Diff(want, got); diff != "" { - t.Fatalf("buildOcAdmReleaseNewCommand() mismatch (-want +got):\n%s", diff) +fi`, + }, } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := buildOcAdmReleaseNewCommand(tt.config, "test-ns", "stable", "cvo-pullspec", "dest:tag", "0.0.1-ver") + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Fatalf("buildOcAdmReleaseNewCommand() mismatch (-want +got):\n%s", diff) + } + }) + } })As per coding guidelines, prefer table-driven tests.
🤖 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/create_release_test.go` around lines 99 - 112, Update the assemble_script test to be table-driven: create a slice of test cases with different ReleaseTagConfiguration names/versions (e.g., "4.10", "4.12", "4.15") and expected script outputs, then iterate and call buildOcAdmReleaseNewCommand for each case, comparing got vs want (use cmp.Diff) and t.Fatalf on mismatch; keep the test runner name ("assemble_script") and reference buildOcAdmReleaseNewCommand and the ReleaseTagConfiguration/SourceTagReferencePolicy setup so each case constructs the config correctly and validates the shell-based command generation across versions.
🤖 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.
Nitpick comments:
In `@pkg/api/promotion.go`:
- Around line 110-112: The comment for getQuayProxyTarget is outdated (mentions
a "-quay" suffix) — update the function docstring to reflect the actual format
produced by getQuayProxyTarget (i.e., "namespace/imagestream-name:tag" or
"namespace/name:tag") and remove the "-quay" mention; ensure the doc references
the ImageStreamTagReference parameter and clearly shows the exact output format
used by the function.
In `@pkg/steps/release/create_release_test.go`:
- Around line 99-112: Update the assemble_script test to be table-driven: create
a slice of test cases with different ReleaseTagConfiguration names/versions
(e.g., "4.10", "4.12", "4.15") and expected script outputs, then iterate and
call buildOcAdmReleaseNewCommand for each case, comparing got vs want (use
cmp.Diff) and t.Fatalf on mismatch; keep the test runner name
("assemble_script") and reference buildOcAdmReleaseNewCommand and the
ReleaseTagConfiguration/SourceTagReferencePolicy setup so each case constructs
the config correctly and validates the shell-based command generation across
versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f05d85fa-a6ae-4438-a5cc-cf41973c69f9
📒 Files selected for processing (10)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_digest.yaml
💤 Files with no reviewable changes (3)
- pkg/defaults/defaults_test.go
- pkg/defaults/defaults.go
- pkg/steps/release/create_release.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, 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 |
|
/test lint validate-prow pod timeout failure |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Scheduling tests matching the |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
deepsm007
force-pushed
the
revert-dptp-4340-gate-4.12
branch
2 times, most recently
from
f2fc9a5 to
5e4f0a4
Compare
deepsm007
changed the title
openshift-ci-robot
removed
the
jira/valid-reference
|
@deepsm007: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/api/promotion.go (2)
131-133: ⚡ Quick winAdd a Go doc comment for exported API.
ConsolidatedQuayPromotionReleaseVersionis exported and should have a proper doc comment for package consumers.Suggested patch
-func ConsolidatedQuayPromotionReleaseVersion(name string) bool { +// ConsolidatedQuayPromotionReleaseVersion reports whether a release stream name +// is eligible for consolidated quay promotion. +func ConsolidatedQuayPromotionReleaseVersion(name string) bool { return consolidatedQuayPromotionVersion(name) }As per coding guidelines, "Go documentation on Classes/Functions/Fields should be written properly" and "Comment important exported functions with their purpose, parameters, and return values."
🤖 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/api/promotion.go` around lines 131 - 133, Add a Go doc comment for the exported function ConsolidatedQuayPromotionReleaseVersion describing its purpose, the parameter (name string) and the boolean return (what true/false indicates); mention that it delegates to consolidatedQuayPromotionVersion(name) so readers understand it forwards the check to that internal helper. Place the comment immediately above the ConsolidatedQuayPromotionReleaseVersion declaration following Go doc conventions (starts with the function name).
147-154: ⚡ Quick winUpdate stale format comment to match conditional suffix behavior.
quayProxyStreamSuffixnow omits-quayfor consolidated versions, so thegetQuayProxyTargetformat comment is no longer always accurate.Suggested patch
-// getQuayProxyTarget creates the quay-proxy target imagestream tag reference. -// Format: namespace/imagestream-name-quay:tag +// getQuayProxyTarget creates the quay-proxy target imagestream tag reference. +// Format: namespace/imagestream-name[:tag], with "-quay" suffix for legacy streams only.As per coding guidelines, "Comments should be useful and explain why code exists."
🤖 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/api/promotion.go` around lines 147 - 154, Update the stale comment on getQuayProxyTarget to reflect that the quayProxyStreamSuffix (used by consolidatedQuayPromotionVersion and quayProxyStreamSuffix) conditionally omits "-quay" for consolidated versions; change the format description to indicate the suffix is conditional (e.g., "namespace/imagestream-name[ -quay-if-not-consolidated]:tag") and reference the consolidatedQuayPromotionVersion/quayProxyStreamSuffix behavior so readers know why the "-quay" may be absent.
🤖 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_promotion_quay_multiple_tags.yaml`:
- Around line 15-17: The three retry loops that run oc tag (the for r in {1..3}
... oc tag ... && break patterns) silently succeed even if all attempts fail
because set +e prevents exiting on error; modify each loop to capture the oc tag
exit status and, after the loop, fail fast if tagging never succeeded (e.g., set
a success flag or store the last exit code from oc tag and if non-zero call exit
1 or return a non-zero error), or alternatively enable errexit for the block —
ensure you update the three tag blocks (the oc tag invocations for
ovn-kubernetes, ovn-kubernetes-base, and ovn-kubernetes-microshift) so that
exhausted retries cause the script/pod to exit non-zero to trigger correct
cleanup on failure.
---
Nitpick comments:
In `@pkg/api/promotion.go`:
- Around line 131-133: Add a Go doc comment for the exported function
ConsolidatedQuayPromotionReleaseVersion describing its purpose, the parameter
(name string) and the boolean return (what true/false indicates); mention that
it delegates to consolidatedQuayPromotionVersion(name) so readers understand it
forwards the check to that internal helper. Place the comment immediately above
the ConsolidatedQuayPromotionReleaseVersion declaration following Go doc
conventions (starts with the function name).
- Around line 147-154: Update the stale comment on getQuayProxyTarget to reflect
that the quayProxyStreamSuffix (used by consolidatedQuayPromotionVersion and
quayProxyStreamSuffix) conditionally omits "-quay" for consolidated versions;
change the format description to indicate the suffix is conditional (e.g.,
"namespace/imagestream-name[ -quay-if-not-consolidated]:tag") and reference the
consolidatedQuayPromotionVersion/quayProxyStreamSuffix behavior so readers know
why the "-quay" may be absent.
🪄 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: e2685d9b-0bab-469b-9bae-a87ca8a23668
📒 Files selected for processing (8)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
deepsm007
force-pushed
the
revert-dptp-4340-gate-4.12
branch
from
5e4f0a4 to
a6bb288
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api/promotion.go`:
- Around line 136-145: The consolidatedQuayPromotionVersion function currently
uses fmt.Sscanf(name, "%d.%d", &major, &minor) which allows partial matches like
"4.12.1" to parse; update the parsing to require an exact "<major>.<minor>"
match (e.g. use a regex anchored ^(\d+)\.(\d+)$ or split on '.' and validate
there are exactly two numeric parts with strconv.Atoi) so only fully-formed
versions are accepted; keep the rest of the function logic (compare major to
consolidatedQuayPromotionMajor and minor to
consolidatedQuayPromotionMinMinor/consolidatedQuayPromotionMaxMinor) unchanged
and return false on any parse failure or extra trailing characters.
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml`:
- Line 14: Replace single-quoted echo strings that contain the loop variable
(e.g. echo 'Tag attempt $r (all together)' and the other occurrences with '$r'
in their messages) with double-quoted strings so the shell expands $r at
runtime; locate the echo invocations in the for-loop retry blocks (the lines
printing "Tag attempt $r ..." and other retry messages) and change their quoting
to allow variable interpolation.
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml`:
- Line 13: The echo statements inside the retry loops use single quotes so the
shell variable $r is not expanded; update each echo (e.g., the echo that
currently reads 'Tag attempt $r (all together)' and the other two similar echo
lines) to use double quotes so $r is interpolated (e.g., "Tag attempt $r ..."),
ensuring retry counts are logged correctly.
🪄 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: 6fb2e661-d7d9-4040-87e2-57d86e557627
📒 Files selected for processing (11)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/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
✅ Files skipped from review due to trivial changes (1)
- pkg/steps/release/promote_test.go
| func consolidatedQuayPromotionVersion(name string) bool { | ||
| var major, minor int | ||
| if _, err := fmt.Sscanf(name, "%d.%d", &major, &minor); err != nil { | ||
| return false | ||
| } | ||
| if major != consolidatedQuayPromotionMajor { | ||
| return false | ||
| } | ||
| return gotMajor == major && gotMinor == minor | ||
| return minor >= consolidatedQuayPromotionMinMinor && minor <= consolidatedQuayPromotionMaxMinor | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, with fmt.Sscanf and format "%d.%d", does scanning "4.12.1" or "4.12-foo" still return two successful assignments while leaving trailing input?
💡 Result:
Answer: No—both inputs will still successfully assign two values, and scanning does not require consuming the entire input. However, whether the second %d succeeds depends on what follows the first “.%d” component. For fmt.Sscanf with format "%d.%d": 1) Input "4.12.1" - First %d (%d) matches "4" and the literal '.' in the format matches the '.' after it. - Second %d matches "12" and stops when it encounters the next '.' (a non-digit for an integer scan). - Sscanf can stop before consuming all input (“need not consume its entire input”), so the remaining ".1" is left unconsumed. - Result: n == 2 (two successful assignments) and err == nil. 2) Input "4.12-foo" - First %d matches "4"; the '.' literal matches the '.' after it. - Second %d matches "12" and stops when it encounters '-' (not a digit for an integer scan). - Again, Sscanf need not consume its entire input, so the remaining "-foo" is left unconsumed. - Result: n == 2 (two successful assignments) and err == nil. Key rules from Go docs: - Sscanf returns the number of items successfully parsed, and it “need not consume its entire input.” [1] - Also, for Scanf/Sscanf, “text in the format string must match the input text; scanning stops if it does not,” and the return value indicates how many arguments were scanned. [2] Implication for your specific cases: after successfully matching “4” + "." + “12”, the parser is allowed to stop, so extra trailing characters like ".1" or "-foo" do not prevent the two assignments from succeeding. [1][2]
Citations:
Use strict parsing for consolidated-quay version gating (avoid fmt.Sscanf partial-match).
fmt.Sscanf(name, "%d.%d", ...) does not require consuming the whole string; values like 4.12.1 and 4.12-foo can still parse as major=4/minor=12 (n==2, err==nil), so they may incorrectly pass the consolidated-quay range checks. Switch to exact <major>.<minor> parsing.
Proposed fix
import (
"fmt"
+ "strconv"
"strings"
@@
func consolidatedQuayPromotionVersion(name string) bool {
- var major, minor int
- if _, err := fmt.Sscanf(name, "%d.%d", &major, &minor); err != nil {
+ parts := strings.Split(name, ".")
+ if len(parts) != 2 {
+ return false
+ }
+ major, err := strconv.Atoi(parts[0])
+ if err != nil {
+ return false
+ }
+ minor, err := strconv.Atoi(parts[1])
+ if err != nil {
return false
}
if major != consolidatedQuayPromotionMajor {
return false
}
return minor >= consolidatedQuayPromotionMinMinor && minor <= consolidatedQuayPromotionMaxMinor
}🤖 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/api/promotion.go` around lines 136 - 145, The
consolidatedQuayPromotionVersion function currently uses fmt.Sscanf(name,
"%d.%d", &major, &minor) which allows partial matches like "4.12.1" to parse;
update the parsing to require an exact "<major>.<minor>" match (e.g. use a regex
anchored ^(\d+)\.(\d+)$ or split on '.' and validate there are exactly two
numeric parts with strconv.Atoi) so only fully-formed versions are accepted;
keep the rest of the function logic (compare major to
consolidatedQuayPromotionMajor and minor to
consolidatedQuayPromotionMinMinor/consolidatedQuayPromotionMaxMinor) unchanged
and return false on any parse failure or extra trailing characters.
| for r in {1..5}; do | ||
| _digest=$(oc image info --registry-config=/etc/push-secret/.dockerconfigjson --filter-by-os=linux/amd64 quay-proxy.ci.openshift.org/openshift/ci:ocp_4.21_ovn-kubernetes | sed -n '/^Digest:[[:space:]]/s/^Digest:[[:space:]]*//p' | head -n1) | ||
| if [ -n "${_digest}" ] && oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@${_digest} ocp/4.21-quay:ovn-kubernetes; then | ||
| for r in {1..2}; do echo 'Tag attempt $r (all together)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ocp/4.21:ovn-kubernetes-base quay-proxy.ci.openshift.org/openshift/ci@sha256:ccc ocp/4.21:ovn-kubernetes-microshift && break; :; done |
There was a problem hiding this comment.
Fix variable interpolation in echo statements.
Lines 14, 17, 30, and 43 use single quotes around messages containing $r, which prevents bash variable expansion. The logs will literally print $r instead of the retry attempt number (1, 2, 3), degrading observability during failure investigation.
🔧 Proposed fix
- for r in {1..2}; do echo 'Tag attempt $r (all together)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ocp/4.21:ovn-kubernetes-base quay-proxy.ci.openshift.org/openshift/ci@sha256:ccc ocp/4.21:ovn-kubernetes-microshift && break; :; done
+ for r in {1..2}; do echo "Tag attempt $r (all together)"; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ocp/4.21:ovn-kubernetes-base quay-proxy.ci.openshift.org/openshift/ci@sha256:ccc ocp/4.21:ovn-kubernetes-microshift && break; :; done
tagged=false
for r in {1..3}; do
- echo 'Tag attempt $r (individual)'
+ echo "Tag attempt $r (individual)"
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes; then
...
tagged=false
for r in {1..3}; do
- echo 'Tag attempt $r (individual)'
+ echo "Tag attempt $r (individual)"
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ocp/4.21:ovn-kubernetes-base; then
...
tagged=false
for r in {1..3}; do
- echo 'Tag attempt $r (individual)'
+ echo "Tag attempt $r (individual)"
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:ccc ocp/4.21:ovn-kubernetes-microshift; thenAlso applies to: 17-17, 30-30, 43-43
🤖 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_multiple_tags.yaml`
at line 14, Replace single-quoted echo strings that contain the loop variable
(e.g. echo 'Tag attempt $r (all together)' and the other occurrences with '$r'
in their messages) with double-quoted strings so the shell expands $r at
runtime; locate the echo invocations in the for-loop retry blocks (the lines
printing "Tag attempt $r ..." and other retry messages) and change their quoting
to allow variable interpolation.
| for r in {1..5}; do | ||
| _digest=$(oc image info --registry-config=/etc/push-secret/.dockerconfigjson --filter-by-os=linux/amd64 quay-proxy.ci.openshift.org/openshift/ci:ocp_4.21_ovn-kubernetes | sed -n '/^Digest:[[:space:]]/s/^Digest:[[:space:]]*//p' | head -n1) | ||
| if [ -n "${_digest}" ] && oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@${_digest} ocp/4.21-quay:ovn-kubernetes; then | ||
| for r in {1..2}; do echo 'Tag attempt $r (all together)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ci/ci-quay:sanitize-prow-jobs quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes && break; :; done |
There was a problem hiding this comment.
Fix variable interpolation in echo statements.
Lines 13, 16, and 29 use single quotes around messages containing $r, preventing bash variable expansion. Logs will show literal $r instead of retry attempt numbers, making failure diagnosis harder.
🔧 Proposed fix
- for r in {1..2}; do echo 'Tag attempt $r (all together)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ci/ci-quay:sanitize-prow-jobs quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes && break; :; done
+ for r in {1..2}; do echo "Tag attempt $r (all together)"; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ci/ci-quay:sanitize-prow-jobs quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes && break; :; done
tagged=false
for r in {1..3}; do
- echo 'Tag attempt $r (individual)'
+ echo "Tag attempt $r (individual)"
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ci/ci-quay:sanitize-prow-jobs; then
...
tagged=false
for r in {1..3}; do
- echo 'Tag attempt $r (individual)'
+ echo "Tag attempt $r (individual)"
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.21:ovn-kubernetes; thenAlso applies to: 16-16, 29-29
🤖 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_non_release_namespace.yaml`
at line 13, The echo statements inside the retry loops use single quotes so the
shell variable $r is not expanded; update each echo (e.g., the echo that
currently reads 'Tag attempt $r (all together)' and the other two similar echo
lines) to use double quotes so $r is interpolated (e.g., "Tag attempt $r ..."),
ensuring retry counts are logged correctly.
Same 4.12 gate pattern; 4.23 and 5.0 stay on legacy promotion.
deepsm007
force-pushed
the
revert-dptp-4340-gate-4.12
branch
from
a6bb288 to
23e1641
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/api/promotion_test.go (1)
221-235: ⚡ Quick winAdd a direct
5.0case inTestQuayCombinedMirrorFunc.This table now validates legacy suffix behavior for
4.23, but5.0is also excluded by contract. Adding a5.0mirror-key assertion here would protect helper-level boundary behavior from regressions.🤖 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/api/promotion_test.go` around lines 221 - 235, Add a new table case in TestQuayCombinedMirrorFunc mirroring the existing "4.23 uses quay suffix" entry but with ImageStreamTagReference.Name set to "5.0" (and adjust any inlined tag strings accordingly); name it e.g. "5.0 uses quay suffix" and add expected mirror-key assertions replacing "4.23" with "5.0" (e.g. "quay.io/openshift/ci:ocp_5.0_ovn-kubernetes", "quay.io/openshift/ci:20241024102030_prune_ocp_5.0_ovn-kubernetes", and "ocp/5.0-quay": "quay-proxy.ci.openshift.org/openshift/ci@sha256:abc123") so the helper-level boundary behavior for 5.0 is asserted.pkg/api/promotion.go (1)
133-135: ⚡ Quick winAdd GoDoc for exported
ConsolidatedQuayPromotionStream.Please document purpose/inputs/return behavior for this exported helper.
Proposed fix
+// ConsolidatedQuayPromotionStream reports whether a release stream name is in the +// consolidated Quay promotion set. func ConsolidatedQuayPromotionStream(name string) bool { return consolidatedQuayPromotionVersion(name) }As per coding guidelines "Comment important exported functions with their purpose, parameters, and return values."
🤖 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/api/promotion.go` around lines 133 - 135, Add a GoDoc comment immediately above the exported function ConsolidatedQuayPromotionStream describing its purpose, the meaning of the input parameter `name` and what the boolean return value represents; ensure the comment starts with "ConsolidatedQuayPromotionStream ..." and explains that it delegates to consolidatedQuayPromotionVersion(name) and when/why it returns true or false.
🤖 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.
Nitpick comments:
In `@pkg/api/promotion_test.go`:
- Around line 221-235: Add a new table case in TestQuayCombinedMirrorFunc
mirroring the existing "4.23 uses quay suffix" entry but with
ImageStreamTagReference.Name set to "5.0" (and adjust any inlined tag strings
accordingly); name it e.g. "5.0 uses quay suffix" and add expected mirror-key
assertions replacing "4.23" with "5.0" (e.g.
"quay.io/openshift/ci:ocp_5.0_ovn-kubernetes",
"quay.io/openshift/ci:20241024102030_prune_ocp_5.0_ovn-kubernetes", and
"ocp/5.0-quay": "quay-proxy.ci.openshift.org/openshift/ci@sha256:abc123") so the
helper-level boundary behavior for 5.0 is asserted.
In `@pkg/api/promotion.go`:
- Around line 133-135: Add a GoDoc comment immediately above the exported
function ConsolidatedQuayPromotionStream describing its purpose, the meaning of
the input parameter `name` and what the boolean return value represents; ensure
the comment starts with "ConsolidatedQuayPromotionStream ..." and explains that
it delegates to consolidatedQuayPromotionVersion(name) and when/why it returns
true or false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0c2754ff-b3f8-421e-87c4-5bc93ea8e3fa
📒 Files selected for processing (10)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote_test.gopkg/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
|
@deepsm007: 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. |
3 participants
Summary
Expands the consolidated Quay promotion rollout from 4.12 only to 4.11 through 4.22, while 4.23 and 5.0 stay on the previous behavior to limit disruption on the newest streams.
For streams in the rollout:
ocp/<version>:<component>without a-quayimagestream suffix).oc adm release new.For 4.23 and 5.0 (unchanged for now):
ocp/<version>-quay:<component>naming.--from-image-streamonly.Follow-up
referenceRelease/referenceMode: sourceon ci imagestreams (4.11–4.22): Add referenceRelease to ci imagestreams for OCP 4.11–4.22 release#79592/cc @openshift/test-platform
Expand Consolidated Quay Promotion to OCP 4.11–4.22
This PR expands the consolidated Quay promotion rollout from 4.12 only to all release streams 4.11 through 4.22, while keeping 4.23 and 5.0 on the previous behavior.
Key Changes
Version-range predicate for consolidated behavior:
ConsolidatedQuayPromotionStream()function that returns true for release versions 4.11–4.22 and false for 4.23, 5.0, and other versionsFor OCP 4.11–4.22 (consolidated promotion):
promotion-quayruns; images are mirrored to Quay and tagged via quay-proxy intoocp/<version>:<component>(no-quaysuffix)--from-image-stream-file(with fallback to--from-image-stream) when building release payloads withoc adm release newFor OCP 4.23 and 5.0 (legacy promotion):
promotionandpromotion-quaysteps continue to runocp/<version>-quay:<component>(retains-quaysuffix)--from-image-streamonlyAffected Components
buildOcAdmReleaseNewCommand()function now branches based on the consolidated version range to select the appropriateoc adm release newinvocation strategyquayProxyStreamSuffix()function now determines whether to use-quaysuffix based on the consolidated version-range predicateTesting
Test fixtures and expectations updated across multiple files to verify:
ocp/<version>:<component>tagging without-quaysuffixocp/<version>-quay:<component>format--reference-mode=source --keep-manifest-listflags