DPTP-3787: promotion: unify ocp quay promotion; drop consolidation and -quay suffix#5233
DPTP-3787: promotion: unify ocp quay promotion; drop consolidation and -quay suffix#5233deepsm007 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughReplaces consolidated-promotion predicates with direct official-image namespace checks, removes consolidation helpers, updates quay-proxy target naming (drops ChangesQuay Promotion Logic Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
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: 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. |
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
|
Scheduling tests matching the |
|
/hold |
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
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
|
/retest-required |
|
/test images e2e |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@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. |
/cc @openshift/test-platform
/hold
Summary
This PR unifies and simplifies how ci-operator decides where to promote images (Quay vs the default registry) by removing the old "consolidated quay promotion" version-based logic and relying on explicit API checks that detect official OCP image namespaces.
What changed (practical impact for CI users/operators)
-quaysuffixing convention were removed; Quay routing is determined by the registry constant (api.QuayOpenShiftCIRepo) and by API helpers that detect official images.oc adm release new) now tries exporting an ImageStream to a temp YAML and uses--from-image-stream-filewhen possible, with a fallback to--from-image-stream. This reduces fragile version-string branching in the release assembly script.ocp/<image>:<tag>rather than relying on-quaysuffixed names.promotionsemantics (e.g., retry logs now use thepromotion:prefix).Components affected
Why this matters
Notes for reviewers/operators
-quaynaming or the consolidated-version predicates is updated to use the new API checks or registry constant.