DPTP-3787: promotion: unify ocp quay promotion; drop consolidation and -quay suffix#5233
Open
deepsm007 wants to merge 1 commit into
Open
DPTP-3787: promotion: unify ocp quay promotion; drop consolidation and -quay suffix#5233deepsm007 wants to merge 1 commit into
deepsm007 wants to merge 1 commit into
Conversation
Contributor
|
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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR updates Quay proxy key formatting, switches several promotion and image-resolution checks to official-image predicates, changes release command generation to prefer image-stream files, and adjusts related tests and retry-shell log output. ChangesOfficial image promotion rewrite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 15 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/api/promotion.go (1)
116-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback rewrites tag-based proxy destinations to the wrong ISTag.
For targets like
quay.io/openshift/ci:ocp_ovn-kubernetes_latest, this parse falls through and Line 132 emitsocp/ovn-kubernetes:ovn-kubernetes. That drops the configured promotion tag (latest) and sends the quay-proxy tag to a different destination than the normal registry promotion path. Please preserve the suffix encoded intarget, or pass the configured promotion tag explicitly instead of defaulting totag.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 116 - 132, The fallback return builds the ISTag using tag.Tag (producing e.g. "ocp/ovn-kubernetes:ovn-kubernetes") which drops the suffix encoded in target; change the fallback logic in the tag-based promotion parsing (the block using variables target, targetParts, tagPart, tagSuffix, targetNamespace, targetComponent) to preserve the suffix from the original target string (e.g. the "_latest" part) or to explicitly use the configured promotion suffix parsed from target (not tag.Tag) when constructing the final fmt.Sprintf("%s/%s:%s"); ensure the code extracts and passes that suffix (or full right-hand portion after the colon) into the returned ISTag instead of defaulting to tag.Tag.pkg/steps/utils/image.go (1)
89-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the
stableshortcut version-aware.This now rewrites any official
ocp/<stream>:<tag>tojobNamespace/stable:<tag>wheneverstablehas that tag. For versioned inputs likeocp/5.0:cli, that can silently pull the current stable payload instead of the requested stream. Please keep the stable fast-path behind a stream/version check and add a regression test wherestable:cliexists but the requested base is a different official stream.As per coding guidelines, "New or modified functionality should include test coverage. Bug fixes should include a regression test that fails without the fix."
🤖 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/utils/image.go` around lines 89 - 103, The stable-image fast-path currently rewrites any official image (base) to jobNamespace/stable:<tag> whenever that tag exists; restrict this by only taking the stable shortcut when the requested base actually refers to the same "stable" stream/version: in the block using api.RefersToOfficialImage, add a guard that the base's stream equals api.StableImageStream (or otherwise confirms the requested stream/version matches the stable stream) before returning the ImageStreamTag from stable; keep the existing ResolvePullSpec logic and error handling but skip the shortcut when base is a versioned/other official stream (e.g., ocp/5.0:cli), and add a regression test that creates stable:cli but requests ocp/5.0:cli to assert the code does not rewrite to jobNamespace/stable:cli.
🤖 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.
Outside diff comments:
In `@pkg/api/promotion.go`:
- Around line 116-132: The fallback return builds the ISTag using tag.Tag
(producing e.g. "ocp/ovn-kubernetes:ovn-kubernetes") which drops the suffix
encoded in target; change the fallback logic in the tag-based promotion parsing
(the block using variables target, targetParts, tagPart, tagSuffix,
targetNamespace, targetComponent) to preserve the suffix from the original
target string (e.g. the "_latest" part) or to explicitly use the configured
promotion suffix parsed from target (not tag.Tag) when constructing the final
fmt.Sprintf("%s/%s:%s"); ensure the code extracts and passes that suffix (or
full right-hand portion after the colon) into the returned ISTag instead of
defaulting to tag.Tag.
In `@pkg/steps/utils/image.go`:
- Around line 89-103: The stable-image fast-path currently rewrites any official
image (base) to jobNamespace/stable:<tag> whenever that tag exists; restrict
this by only taking the stable shortcut when the requested base actually refers
to the same "stable" stream/version: in the block using
api.RefersToOfficialImage, add a guard that the base's stream equals
api.StableImageStream (or otherwise confirms the requested stream/version
matches the stable stream) before returning the ImageStreamTag from stable; keep
the existing ResolvePullSpec logic and error handling but skip the shortcut when
base is a versioned/other official stream (e.g., ocp/5.0:cli), and add a
regression test that creates stable:cli but requests ocp/5.0:cli to assert the
code does not rewrite to jobNamespace/stable:cli.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 00289a20-81ca-425d-bc71-e6b805920291
📒 Files selected for processing (17)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/input_image_tag_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/snapshot.gopkg/steps/release/snapshot_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/utils/image.gopkg/steps/utils/image_test.go
💤 Files with no reviewable changes (1)
- pkg/steps/release/create_release.go
deepsm007
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@deepsm007: This pull request references DPTP-3787 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. |
deepsm007
force-pushed
the
revert/consolidated-quay-promotion
branch
from
5010fbf to
2b0b091
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/defaults/defaults.go`:
- Around line 306-314: Two promotion steps are created with the same logical
name (api.PromotionStepName) which leads to duplicate Step.Name() values; update
the second call that builds the Quay promotion step (the
releasesteps.PromotionStep invocation that currently passes
api.PromotionStepName and
api.QuayOpenShiftCIRepo/api.QuayCombinedMirrorFunc/api.QuayTargetNameFunc) to
use a distinct step name (for example introduce or use a constant like
api.QuayPromotionStepName or append a "-quay" suffix) so the formatted
Step.Name() differs from the other promotion node; ensure the new name is passed
where the first param (currently api.PromotionStepName) is supplied and run a
quick grep/audit for any code that assumes step-name uniqueness to confirm no
other changes are required.
🪄 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: 466ad4e3-02bd-490b-8c10-53c909a4613d
📒 Files selected for processing (17)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/input_image_tag_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/snapshot.gopkg/steps/release/snapshot_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/utils/image.gopkg/steps/utils/image_test.go
💤 Files with no reviewable changes (1)
- pkg/steps/release/create_release.go
✅ Files skipped from review due to trivial changes (3)
- pkg/steps/release/snapshot_test.go
- 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 as they are similar to previous changes (10)
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
- pkg/steps/release/snapshot.go
- pkg/steps/release/create_release_test.go
- pkg/defaults/defaults_test.go
- pkg/steps/utils/image.go
- pkg/api/promotion_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
- pkg/steps/release/promote_test.go
- pkg/steps/release/promote.go
- pkg/api/promotion.go
Contributor
|
Scheduling tests matching the |
Contributor
Author
|
/hold |
deepsm007
force-pushed
the
revert/consolidated-quay-promotion
branch
3 times, most recently
from
70b83ec to
b16fe83
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/steps/release/promote.go (1)
292-322:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEligibility logic now keying off
ocp/origin(not consolidated stream versions); tests cover-quay+ocppaths but missorigin.
quayProxyTagFromISKeynow treats a stream as eligible whenapi.RefersToOfficialImage(namespace, api.WithOKD)is true (alwaysocp, andoriginwhen WithOKD) unlessstreamPartends with-quay.TestQuayProxyTagFromISKeyalready covers:
- legacy
-quaysuffix inputs (e.g.ocp/4.21-quay:...,ocp/5.0-quay:...)- namespace-based non-
-quaystream inputs (e.g.ocp/4.13:secondary-scheduler-operator)Add a test for
origin/<streamPart>:<tag>wherestreamPartdoes not end with-quayto exercise theoriginbranch.🤖 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/promote.go` around lines 292 - 322, quayProxyTagFromISKey's branch that accepts non-`-quay` streams for the `origin` namespace (via api.RefersToOfficialImage(namespace, api.WithOKD)) is not covered by tests; add a unit test in TestQuayProxyTagFromISKey that calls quayProxyTagFromISKey with an input like "origin/<streamPart>:<tag>" where <streamPart> does NOT end with "-quay", assert the function returns true and the expected formatted tag string (using api.QCIAPPCIDomain, namespace "origin", the given streamPart and tag), and ensure the test exercises the origin branch rather than the -quay or failure paths.pkg/api/promotion.go (1)
110-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten/cover
getQuayProxyTargetend-of-function fallback assumptions
pkg/api/promotion.go:getQuayProxyTargetalready has test coverage for the “tag-based” parsing paths (e.g.ocp/ovn-kubernetes:latest,ocp/ovn-kubernetes:ci_a_latest) viaTestQuayCombinedMirrorFunc/QuayCombinedMirrorFunc, so the second parsing branch’stag.Tag + "_"prefix extraction matches theQuayTargetNameFunc-generated target format.- The final fallback (
return fmt.Sprintf("%s/%s:%s", tag.Namespace, tag.Tag, tag.Tag)) is not exercised by existing tests; if parsing ever fails to extract the${suffix}part fromtarget, this fallback would assume the ImageStream name and tag are bothtag.Tag, which may not match the intended<component>_<suffix>pattern.🤖 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 - 141, The final fallback in getQuayProxyTarget currently assumes the ImageStream name equals tag.Tag, which is unsafe; change the fallback to extract the component from the original target (reuse targetParts[1] when available) and use that as the ImageStream name with tag.Tag as the tag, falling back to a safe default only if the target cannot be parsed; update getQuayProxyTarget to prefer targetParts[1] (or its component portion) for the image stream name and only use tag.Tag as both name+tag as a last-resort, ensuring you reference getQuayProxyTarget, targetParts, tag.Tag and tag.Namespace when making the change.
🤖 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.
Outside diff comments:
In `@pkg/api/promotion.go`:
- Around line 110-141: The final fallback in getQuayProxyTarget currently
assumes the ImageStream name equals tag.Tag, which is unsafe; change the
fallback to extract the component from the original target (reuse targetParts[1]
when available) and use that as the ImageStream name with tag.Tag as the tag,
falling back to a safe default only if the target cannot be parsed; update
getQuayProxyTarget to prefer targetParts[1] (or its component portion) for the
image stream name and only use tag.Tag as both name+tag as a last-resort,
ensuring you reference getQuayProxyTarget, targetParts, tag.Tag and
tag.Namespace when making the change.
In `@pkg/steps/release/promote.go`:
- Around line 292-322: quayProxyTagFromISKey's branch that accepts non-`-quay`
streams for the `origin` namespace (via api.RefersToOfficialImage(namespace,
api.WithOKD)) is not covered by tests; add a unit test in
TestQuayProxyTagFromISKey that calls quayProxyTagFromISKey with an input like
"origin/<streamPart>:<tag>" where <streamPart> does NOT end with "-quay", assert
the function returns true and the expected formatted tag string (using
api.QCIAPPCIDomain, namespace "origin", the given streamPart and tag), and
ensure the test exercises the origin branch rather than the -quay or failure
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d03f6e12-c67a-4f10-aec3-c2450df8a469
📒 Files selected for processing (16)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/input_image_tag_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/snapshot.gopkg/steps/release/snapshot_test.gopkg/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/utils/image.gopkg/steps/utils/image_test.go
💤 Files with no reviewable changes (1)
- pkg/steps/release/create_release.go
✅ Files skipped from review due to trivial changes (3)
- pkg/steps/release/snapshot_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/steps/release/create_release_test.go
- pkg/api/promotion_test.go
- pkg/steps/utils/image_test.go
- pkg/steps/release/snapshot.go
- pkg/defaults/defaults.go
- pkg/steps/input_image_tag_test.go
- pkg/steps/release/promote_test.go
deepsm007
force-pushed
the
revert/consolidated-quay-promotion
branch
from
b16fe83 to
19a1202
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/promotion.go (1)
117-141: 💤 Low valueParsing logic is correct but dense; a brief why-comment per branch would help.
The two extraction patterns (suffix-match vs
tag.Tag_-prefix) handle distinct target shapes and both branches are exercised byTestQuayCombinedMirrorFunc. The control flow is hard to follow at a glance — a one-line comment on each block explaining the target shape it matches would reduce future maintenance risk. No behavior change needed.🤖 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 117 - 141, Add concise one-line comments explaining each branch of the tag parsing logic around targetParts/tagPart: annotate the first branch (where tagSuffix is used and targetComponent is extracted) with a comment like "matches targets where component name appears between namespace and suffix '_<tag>' (e.g. namespace/component_tag)" and annotate the second branch (where componentPrefix is detected from tag.Tag+"_" and remainder is used) with a comment like "matches targets where tag is used as namespace and component follows with '<tag>_component' pattern"; reference the existing locals targetParts, tagPart, tagSuffix, targetComponent, remainder, componentPrefix and the test TestQuayCombinedMirrorFunc to indicate these patterns are exercised.
🤖 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 117-141: Add concise one-line comments explaining each branch of
the tag parsing logic around targetParts/tagPart: annotate the first branch
(where tagSuffix is used and targetComponent is extracted) with a comment like
"matches targets where component name appears between namespace and suffix
'_<tag>' (e.g. namespace/component_tag)" and annotate the second branch (where
componentPrefix is detected from tag.Tag+"_" and remainder is used) with a
comment like "matches targets where tag is used as namespace and component
follows with '<tag>_component' pattern"; reference the existing locals
targetParts, tagPart, tagSuffix, targetComponent, remainder, componentPrefix and
the test TestQuayCombinedMirrorFunc to indicate these patterns are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8dfa51f1-e97a-4e4a-b069-a9ff7cced1a9
📒 Files selected for processing (16)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/input_image_tag_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/snapshot.gopkg/steps/release/snapshot_test.gopkg/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/utils/image.gopkg/steps/utils/image_test.go
💤 Files with no reviewable changes (1)
- pkg/steps/release/create_release.go
✅ Files skipped from review due to trivial changes (2)
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
- pkg/steps/release/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/steps/release/snapshot_test.go
- pkg/api/promotion_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
- pkg/steps/release/create_release_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
- pkg/steps/utils/image_test.go
- pkg/defaults/defaults.go
- pkg/defaults/defaults_test.go
- pkg/steps/release/promote_test.go
- pkg/steps/input_image_tag_test.go
- pkg/steps/release/promote.go
Member
|
/retest-required |
Contributor
Author
|
/test images e2e |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
deepsm007
force-pushed
the
revert/consolidated-quay-promotion
branch
from
19a1202 to
6040116
Compare
deepsm007
force-pushed
the
revert/consolidated-quay-promotion
branch
from
6040116 to
145f1b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 475-479: The malformed-tag guard in getResolveAndTagRetryShell
uses a different log prefix than the rest of the function, which breaks
consistent log filtering. Update the echo message in getResolveAndTagRetryShell
to use the same unified promotion: prefix as the other failure/retry messages,
while keeping the same malformed quay proxy tag handling and exit behavior.
🪄 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: 643b4331-5fac-46fe-b810-ecbc697f53bb
📒 Files selected for processing (16)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/input_image_tag_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/snapshot.gopkg/steps/release/snapshot_test.gopkg/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.yamlpkg/steps/utils/image.gopkg/steps/utils/image_test.go
🔗 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 due to trivial changes (3)
- pkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_non_release_namespace.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_multiple_tags.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_4.12.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/steps/utils/image_test.go
- pkg/steps/release/create_release.go
- pkg/defaults/defaults.go
- pkg/defaults/defaults_test.go
- pkg/steps/release/create_release_test.go
- pkg/api/promotion.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 475-479: The malformed-tag guard in getResolveAndTagRetryShell
uses a different log prefix than the rest of the function, which breaks
consistent log filtering. Update the echo message in getResolveAndTagRetryShell
to use the same unified promotion: prefix as the other failure/retry messages,
while keeping the same malformed quay proxy tag handling and exit behavior.
🪄 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: 643b4331-5fac-46fe-b810-ecbc697f53bb
📒 Files selected for processing (16)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/input_image_tag_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/snapshot.gopkg/steps/release/snapshot_test.gopkg/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.yamlpkg/steps/utils/image.gopkg/steps/utils/image_test.go
🔗 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 due to trivial changes (3)
- pkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_non_release_namespace.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_multiple_tags.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetQuayPromotionShell_promotion_quay_4.12.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/steps/utils/image_test.go
- pkg/steps/release/create_release.go
- pkg/defaults/defaults.go
- pkg/defaults/defaults_test.go
- pkg/steps/release/create_release_test.go
- pkg/api/promotion.go
🛑 Comments failed to post (1)
pkg/steps/release/promote.go (1)
475-479: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Inconsistent log prefix in the malformed-tag guard.
The failure/retry echoes in this same function use the unified
promotion:prefix, but this guard still emitspromotion-quay:. Align it for consistent log filtering.Proposed fix
- return fmt.Sprintf("echo promotion-quay: malformed quay proxy tag %q >&2\nexit 1", quayProxyTag) + return fmt.Sprintf("echo promotion: malformed quay proxy tag %q >&2\nexit 1", quayProxyTag)📝 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.func getResolveAndTagRetryShell(registryConfig, quayProxyTag, isTag string, loglevel int, filterByOS string) string { colon := strings.LastIndex(quayProxyTag, ":") if colon == -1 { return fmt.Sprintf("echo promotion: malformed quay proxy tag %q >&2\nexit 1", quayProxyTag) }🤖 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/promote.go` around lines 475 - 479, The malformed-tag guard in getResolveAndTagRetryShell uses a different log prefix than the rest of the function, which breaks consistent log filtering. Update the echo message in getResolveAndTagRetryShell to use the same unified promotion: prefix as the other failure/retry messages, while keeping the same malformed quay proxy tag handling and exit behavior.
Contributor
|
@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. |
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
3 participants
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.
/cc @openshift/test-platform
/hold
This PR simplifies OpenShift image promotion in ci-operator by removing the old “consolidated quay promotion” split and making promotion behavior depend on whether a target refers to an official OCP/OKD image stream.
Practically, CI promotion now uses a single
promotionpath for official images, while Quay-specific behavior is driven by image namespace/tag detection instead of version-based consolidation checks. That means:ocp/<name>:<tag>style instead of-quaysuffixed keyspromotion:prefixThe PR also removes obsolete helpers tied to consolidated promotion and updates tests/fixtures across promotion, release, and image-resolution code to match the new naming and selection behavior.