fix(release): quay promotion in Go#5255
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (12)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughPromotion registration now skips the default step for consolidated Quay promotion, adds skipped-image filtering to promoted tag selection, and routes Quay promotion through a dedicated script-backed pod path. Tests and fixtures were updated for the new Quay shell and pod outputs. ChangesPromotion step gating, skipped-image filtering, and Quay split
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
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: 1
🧹 Nitpick comments (2)
pkg/steps/release/quay_promotion.go (2)
574-586: 💤 Low valueString-based JSON parsing is fragile.
parseImageInfoDigestuses substring matching rather thanjson.Unmarshal. This works for simple cases but could break if the JSON contains escaped quotes within the digest value (unlikely but possible) or if the output format changes.Consider using proper JSON unmarshaling for robustness:
Alternative implementation
func parseImageInfoDigest(output string) (string, bool) { var info struct { Digest string `json:"digest"` } if err := json.Unmarshal([]byte(output), &info); err != nil || info.Digest == "" { return "", false } return info.Digest, true }🤖 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/quay_promotion.go` around lines 574 - 586, The parseImageInfoDigest function uses fragile string-based JSON parsing with substring matching (looking for the prefix "digest":" and finding the closing quote) which can break if the JSON contains escaped quotes within the digest value or if the output format changes. Replace this with proper JSON unmarshaling by defining a struct with a Digest field tagged with json:"digest", then use json.Unmarshal to parse the output string into this struct, returning the Digest field value and a bool indicating success (checking both for unmarshal errors and empty digest values).
485-494: ⚖️ Poor tradeoffMissing resource limits and security context on container.
Per coding guidelines, Kubernetes containers should have resource limits (
cpu,memory) and security hardening (runAsNonRoot,readOnlyRootFilesystem: falsesince KUBECACHEDIR writes to /tmp,allowPrivilegeEscalation: false). This pod runs in CI namespaces but should still follow best practices.Suggested addition
Containers: []coreapi.Container{{ Name: quayPromotionContainer, Image: cliImage, Command: []string{"sleep", fmt.Sprintf("%d", quayPromotionPodSleepSec)}, Env: []coreapi.EnvVar{ {Name: "KUBECONFIG", Value: promotionPodKubeconfigPath}, {Name: "KUBECACHEDIR", Value: "/tmp/.kube/cache"}, }, Resources: coreapi.ResourceRequirements{ Requests: coreapi.ResourceList{ coreapi.ResourceCPU: resource.MustParse("100m"), coreapi.ResourceMemory: resource.MustParse("256Mi"), }, Limits: coreapi.ResourceList{ coreapi.ResourceCPU: resource.MustParse("500m"), coreapi.ResourceMemory: resource.MustParse("512Mi"), }, }, SecurityContext: &coreapi.SecurityContext{ AllowPrivilegeEscalation: ptr.To(false), RunAsNonRoot: ptr.To(true), Capabilities: &coreapi.Capabilities{Drop: []coreapi.Capability{"ALL"}}, }, VolumeMounts: mounts, }},🤖 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/quay_promotion.go` around lines 485 - 494, Add resource limits and security hardening to the quayPromotionContainer in the Containers array. Add a Resources field with cpu and memory requests (100m cpu, 256Mi memory) and limits (500m cpu, 512Mi memory), and add a SecurityContext field configured with AllowPrivilegeEscalation set to false, RunAsNonRoot set to true, and Capabilities with all capabilities dropped. These additions follow Kubernetes best practices for pod security and resource management.Source: Coding guidelines
🤖 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/quay_promotion.go`:
- Around line 273-276: The resolveAndTag method in quayPromotionRunner has a
potential panic because strings.LastIndex returns -1 when the colon substring is
not found in quayProxyTag, and using -1 as a slice bound causes a bounds panic.
Add a defensive check after the strings.LastIndex call to verify the returned
index is not -1; if it is, return an error indicating that quayProxyTag is
malformed (missing the expected colon separator). This ensures invalid input
cannot cause the function to panic.
---
Nitpick comments:
In `@pkg/steps/release/quay_promotion.go`:
- Around line 574-586: The parseImageInfoDigest function uses fragile
string-based JSON parsing with substring matching (looking for the prefix
"digest":" and finding the closing quote) which can break if the JSON contains
escaped quotes within the digest value or if the output format changes. Replace
this with proper JSON unmarshaling by defining a struct with a Digest field
tagged with json:"digest", then use json.Unmarshal to parse the output string
into this struct, returning the Digest field value and a bool indicating success
(checking both for unmarshal errors and empty digest values).
- Around line 485-494: Add resource limits and security hardening to the
quayPromotionContainer in the Containers array. Add a Resources field with cpu
and memory requests (100m cpu, 256Mi memory) and limits (500m cpu, 512Mi
memory), and add a SecurityContext field configured with
AllowPrivilegeEscalation set to false, RunAsNonRoot set to true, and
Capabilities with all capabilities dropped. These additions follow Kubernetes
best practices for pod security and resource management.
🪄 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: e6aaa0e5-336f-4f00-bab0-1449d7acd2b8
📒 Files selected for processing (10)
pkg/defaults/defaults.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/quay_promote_test.gopkg/steps/release/quay_promotion.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.yamlpkg/steps/release/testdata/zz_fixture_TestQuayPromotionPod.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 with no reviewable changes (4)
- 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
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
deepsm007
force-pushed
the
fix/promotion-quay-go
branch
3 times, most recently
from
193b133 to
72c356f
Compare
|
/test e2e |
|
/test all |
|
/override ci/prow/e2e |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/e2e 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 kubernetes-sigs/prow repository. |
|
/test integration |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/assign @danilo-gemoli |
deepsm007
force-pushed
the
fix/promotion-quay-go
branch
from
72c356f to
cf9d400
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/promote.go`:
- Around line 412-415: The deferred ConfigMap cleanup in promote.go reuses the
run context, so if RunPod cancels or times out, s.client.Delete may never
complete and the ConfigMap can leak. Update the deferred cleanup to use a
detached short-lived background context for the Delete call, while keeping the
existing logging with configMap.Name and the error handling in place.
- Around line 467-468: The tag-loop generation in
getTagLoopCommand/tagRetryShell is letting shell expansion alter template
destinations containing ${component}. Update the generated shell content so
those tag strings are emitted literally, either by quoting the heredoc delimiter
in the tag-loop helper or escaping $ in the entries before appending them in
promote.go. Make sure the tags passed through the len(tags) > 0 path remain
unchanged when /bin/sh runs newPromotionPod.
🪄 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: 9fec487f-1441-4738-a279-25629e2f6cbd
📒 Files selected for processing (12)
pkg/defaults/defaults.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.yamlpkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionPod.yamlpkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_4.12.yamlpkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_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 with no reviewable changes (4)
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
✅ Files skipped from review due to trivial changes (1)
- pkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionPod.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/defaults/defaults.go
Run promotion-quay as a shell script in a ConfigMap-mounted pod via RunPod. Consolidates promotion pod helpers with [promotion], preserves build_if_affected skip and existing mirror/tag shell logic.
deepsm007
force-pushed
the
fix/promotion-quay-go
branch
from
cf9d400 to
d1b0457
Compare
|
/test e2e |
|
/override ci/prow/images |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/integration 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 kubernetes-sigs/prow repository. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/images 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 kubernetes-sigs/prow repository. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest |
|
/test e2e |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, deepsm007 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 |
|
/override ci/prow/e2e |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/e2e 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 kubernetes-sigs/prow repository. |
|
@deepsm007: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
/hold
/cc @danilo-gemoli
Move [promotion-quay] to a ConfigMap-mounted shell script pod (same mirror/tag logic as today), keep [promotion] unchanged, and skip promoting images that build_if_affected didn't build.
This updates the release promotion step so
[promotion-quay]now runs as a Go-based implementation instead of the old bash-based pod, while keeping the same promotion routing and Quay-specific behavior separate from the standard[promotion]flow.For CI users, the practical effect is that promotion jobs now build the right set of images more intelligently: optional tool images can be skipped when
build_if_affectedis enabled, but required images still force promotion. The Quay promotion path is also isolated into its own execution and pod/script generation, which simplifies the generic promotion pod and removes the old Quay shell logic from the default path.The tests and fixtures were updated to cover the new Go Quay promotion pod/script generation, the skipped-image behavior, and the revised promotion pod outputs. Several old Quay promotion YAML fixtures were removed and replaced with new ones matching the Go implementation.