Skip to content

DPTP-3787: promotion: unify ocp quay promotion; drop consolidation and -quay suffix#5233

Open
deepsm007 wants to merge 1 commit into
openshift:mainfrom
deepsm007:revert/consolidated-quay-promotion
Open

DPTP-3787: promotion: unify ocp quay promotion; drop consolidation and -quay suffix#5233
deepsm007 wants to merge 1 commit into
openshift:mainfrom
deepsm007:revert/consolidated-quay-promotion

Conversation

@deepsm007

@deepsm007 deepsm007 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

/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)

  • Promotion destination is now chosen automatically from image/namespace metadata instead of special-version string checks. Jobs do not need special version-based configuration to promote to Quay.
  • The separate "consolidated" helpers and the previous -quay suffixing convention were removed; Quay routing is determined by the registry constant (api.QuayOpenShiftCIRepo) and by API helpers that detect official images.
  • Release creation (oc adm release new) now tries exporting an ImageStream to a temp YAML and uses --from-image-stream-file when possible, with a fallback to --from-image-stream. This reduces fragile version-string branching in the release assembly script.
  • Quay-proxy mapping and snapshot/resolution now use explicit namespace/image checks (api.PromotesOfficialImages and api.RefersToOfficialImage) and construct targets like ocp/<image>:<tag> rather than relying on -quay suffixed names.
  • Log/fixture strings and test expectations were adjusted to reflect the unified promotion semantics (e.g., retry logs now use the promotion: prefix).

Components affected

  • ci-operator promotion step (promotion routing and step selection)
  • Release payload creation code (oc adm release command generation)
  • Image snapshot/resolution and resolver helpers
  • Quay proxy target mapping and related tests/fixtures

Why this matters

  • Simplifies configuration: CI jobs that build official OCP images no longer need special version-flagging to get Quay promotion.
  • Makes promotion behavior more robust and easier to reason about by deriving intent from namespaces/APIs rather than hardcoded version lists.
  • Test fixtures and unit tests have been updated to reflect the single, unified promotion flow.

Notes for reviewers/operators

  • Exported consolidation helpers (ConsolidatedQuayPromotion, ConsolidatedQuayPromotionVersion) were removed—check external callers if any.
  • Verify any external tooling or automation that depended on the old -quay naming or the consolidated-version predicates is updated to use the new API checks or registry constant.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@openshift-ci openshift-ci Bot requested a review from a team June 3, 2026 21:54
@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2026
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Replaces consolidated-promotion predicates with direct official-image namespace checks, removes consolidation helpers, updates quay-proxy target naming (drops -quay), adjusts promotion routing, snapshot/image resolution, release script assembly, and test fixtures.

Changes

Quay Promotion Logic Refactor

Layer / File(s) Summary
API: getQuayProxyTarget and tests
pkg/api/promotion.go, pkg/api/promotion_test.go
Rewrote getQuayProxyTarget to emit ocp/<name>:<tag> or tag.Namespace/tag.Name:tag.Tag formats and removed consolidation helpers; updated tests to expect new mirror keys.
Defaults: promotion step selection
pkg/defaults/defaults.go, pkg/defaults/defaults_test.go
fromConfig now uses PromotesOfficialImages(..., api.WithoutOKD) instead of ConsolidatedQuayPromotion(...); test cases for OCP versions updated to "quay only" and ocp namespace.
Promotion runtime and routing
pkg/steps/release/promote.go, pkg/steps/release/promote_test.go, pkg/steps/release/testdata/*
Quay routing determined from registry == api.QuayOpenShiftCIRepo; quayProxyTagFromISKey uses RefersToOfficialImage(..., api.WithOKD) predicate; retry log prefix changed from promotion-quay: to promotion:; tests and fixtures updated accordingly.
Release creation script assembly
pkg/steps/release/create_release.go, pkg/steps/release/create_release_test.go
buildOcAdmReleaseNewCommand always tries oc get imagestream -o yaml and uses --from-image-stream-file with fallback to --from-image-stream; tests expect --reference-mode=source --keep-manifest-list.
Snapshot import resolution
pkg/steps/release/snapshot.go, pkg/steps/release/snapshot_test.go
snapshotImportSource now chooses official-image path via RefersToOfficialImage(..., api.WithoutOKD) instead of consolidated-version predicate; test labels updated.
ResolveOfficialInputFrom and tests
pkg/steps/utils/image.go, pkg/steps/utils/image_test.go
Simplified gating to RefersToOfficialImage(..., api.WithoutOKD); updated test matrix (added official ocp 5.0 and non-official cases, plus versioned-stream scenario).
Input image tag test updates
pkg/steps/input_image_tag_test.go
Removed legacy stream test, added consolidated-stream test, switched TagReferencePolicy to LocalTagReferencePolicy, and use api.StableImageStream in tests.
Test fixtures: promotion log prefix updates
pkg/steps/release/testdata/*
Updated stderr echo prefixes in promotion fixture scripts from promotion-quay: to promotion: for multiple retry loops.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • openshift/ci-tools#5220: Also modifies pkg/steps/release/snapshot.go decision logic for official/consolidated references.
  • openshift/ci-tools#5216: Also updates quay-proxy promotion logic and related tests/fixtures in pkg/steps/release/promote.go.
  • openshift/ci-tools#4866: Related changes to pkg/api/promotion.go getQuayProxyTarget and removal of -quay infix.

Suggested labels

lgtm

Suggested reviewers

  • bear-redhat
  • danilo-gemoli
  • smg247
🚥 Pre-merge checks | ✅ 16 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (16 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: unifying OCP quay promotion by removing consolidation logic and the -quay suffix, which aligns with the substantial refactoring across multiple files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Go Error Handling ✅ Passed Error handling patterns follow Go best practices: 16 errors wrapped with fmt.Errorf %w, no panics, nil checks before dereferencing, justified underscore ignoring for informational returns.
Test Coverage For New Features ✅ Passed All modified and new functions have comprehensive test coverage: 8 test functions with 45+ total test cases covering logic changes from consolidation-based to namespace-based promotion routing.
Stable And Deterministic Test Names ✅ Passed All test names in modified test files use stable, descriptive static strings with no dynamic content (pod names, timestamps, UUIDs, node names, or random identifiers).
Test Structure And Quality ✅ Passed PR contains no Ginkgo tests. The codebase uses only standard Go testing (testing package). Custom check for Ginkgo test quality is not applicable.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests are added in this PR. All test modifications are unit tests using the standard Go testing framework (func TestXxx(t *testing.T)). The check is not applicable.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PR adds no Ginkgo e2e tests. All 39 modified test functions use standard Go testing.T framework (unit tests), not e2e Ginkgo patterns. SNO compatibility check does not apply.
Topology-Aware Scheduling Compatibility ✅ Passed PR modifies image promotion logic and tests only; no deployment manifests, Pods, or scheduling constraints are introduced.
Ote Binary Stdout Contract ✅ Passed PR modifies ci-tools infrastructure code (promotion logic), not OTE test binaries. OTE contract applies only to test suite runners communicating JSON on stdout.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests added in this PR. All changes are standard Go unit tests and production code refactoring. Check not applicable.
No-Weak-Crypto ✅ Passed No weak cryptography usage (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, custom crypto, or insecure secret comparisons) found in any modified files.
Container-Privileges ✅ Passed PR contains no container/K8s manifests with privileged settings. Changes are Go code refactoring; YAML fixtures are shell test outputs without privilege configs.
No-Sensitive-Data-In-Logs ✅ Passed No new logging statements expose passwords, tokens, API keys, PII, or credentials. Changes only log image tag names, attempt numbers, and timing information.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci

openshift-ci Bot commented Jun 3, 2026

Copy link
Copy Markdown
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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Fallback 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 emits ocp/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 in target, or pass the configured promotion tag explicitly instead of defaulting to tag.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 win

Keep the stable shortcut version-aware.

This now rewrites any official ocp/<stream>:<tag> to jobNamespace/stable:<tag> whenever stable has that tag. For versioned inputs like ocp/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 where stable:cli exists 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

📥 Commits

Reviewing files that changed from the base of the PR and between f99d129 and 5010fbf.

📒 Files selected for processing (17)
  • pkg/api/promotion.go
  • pkg/api/promotion_test.go
  • pkg/defaults/defaults.go
  • pkg/defaults/defaults_test.go
  • pkg/steps/input_image_tag_test.go
  • pkg/steps/release/create_release.go
  • pkg/steps/release/create_release_test.go
  • pkg/steps/release/promote.go
  • pkg/steps/release/promote_test.go
  • pkg/steps/release/snapshot.go
  • pkg/steps/release/snapshot_test.go
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
  • pkg/steps/utils/image.go
  • pkg/steps/utils/image_test.go
💤 Files with no reviewable changes (1)
  • pkg/steps/release/create_release.go

@deepsm007 deepsm007 changed the title promotion: unify ocp quay promotion; drop consolidation and -quay suffix DPTP-3787: promotion: unify ocp quay promotion; drop consolidation and -quay suffix Jun 3, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 3, 2026
@openshift-ci-robot

openshift-ci-robot commented Jun 3, 2026

Copy link
Copy Markdown
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.

Details

In response to this:

/cc @openshift/test-platform

/hold

Unify OCP Quay Promotion and Remove Consolidation Logic

This PR consolidates OpenShift's image promotion workflow by removing the "consolidated quay promotion" implementation and unifying all image promotion to use a single promotion step rather than separate [promotion] and [promotion-quay] steps.

Key Changes

Promotion Logic Simplification:

  • Removes the ConsolidatedQuayPromotion(), ConsolidatedQuayPromotionVersion(), and PromotionQuayStepName helpers that previously determined when to use special quay-promotion behavior
  • Consolidates all promotion under a single step named promotion (removing the -quay suffix convention)
  • Changes the promotion destination logic to use PromotesOfficialImages(cfg.CIConfig, api.WithoutOKD): when false, images promote to the default registry; when true (official OCP images), images promote to Quay

Official Image Detection:

  • Replaces checks on ConsolidatedQuayPromotionVersion with the existing RefersToOfficialImage() API to determine when images should be promoted to Quay (when they belong to the ocp or origin namespaces)
  • This unified approach uses the promotion target configuration's namespace rather than special version strings

Affected Components:

  • ci-operator promotion: The promotion step now uniformly handles both Quay and registry targets based on the target namespace rather than configuration version
  • Release payload creation: oc adm release new commands now use a unified --from-image-stream-file approach instead of version-specific branching
  • Image snapshots and resolving: Official image detection relies on namespace checks rather than special version predicates
  • Quay proxy targeting: Updated to directly use namespace/tag information without suffix manipulation logic

Impact for CI Users and Operators

  • Jobs promoting official OCP images will still be promoted to Quay, but now through a single unified promotion step rather than separate mechanisms
  • Simplified configuration: no more dual promotion steps; the promotion destination (Quay vs registry) is determined automatically based on the target's namespace
  • Reduced complexity in ci-operator's promotion logic, making it easier to understand and maintain

Testing Updates

  • Updated test fixtures to reflect the single promotion step name and new logic
  • Quay proxy tag handling tests updated with new namespace-based resolution expectations
  • Default configuration test cases simplified from dual-step promotion to single-step promotion

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 deepsm007 force-pushed the revert/consolidated-quay-promotion branch from 5010fbf to 2b0b091 Compare June 3, 2026 22:58

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5010fbf and 2b0b091.

📒 Files selected for processing (17)
  • pkg/api/promotion.go
  • pkg/api/promotion_test.go
  • pkg/defaults/defaults.go
  • pkg/defaults/defaults_test.go
  • pkg/steps/input_image_tag_test.go
  • pkg/steps/release/create_release.go
  • pkg/steps/release/create_release_test.go
  • pkg/steps/release/promote.go
  • pkg/steps/release/promote_test.go
  • pkg/steps/release/snapshot.go
  • pkg/steps/release/snapshot_test.go
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
  • pkg/steps/utils/image.go
  • pkg/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

Comment thread pkg/defaults/defaults.go
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e

@deepsm007

Copy link
Copy Markdown
Contributor Author

/hold

@deepsm007 deepsm007 force-pushed the revert/consolidated-quay-promotion branch 3 times, most recently from 70b83ec to b16fe83 Compare June 4, 2026 01:21

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Eligibility logic now keying off ocp/origin (not consolidated stream versions); tests cover -quay + ocp paths but miss origin.

quayProxyTagFromISKey now treats a stream as eligible when api.RefersToOfficialImage(namespace, api.WithOKD) is true (always ocp, and origin when WithOKD) unless streamPart ends with -quay. TestQuayProxyTagFromISKey already covers:

  • legacy -quay suffix inputs (e.g. ocp/4.21-quay:..., ocp/5.0-quay:...)
  • namespace-based non--quay stream inputs (e.g. ocp/4.13:secondary-scheduler-operator)

Add a test for origin/<streamPart>:<tag> where streamPart does not end with -quay to exercise the origin branch.

🤖 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 win

Tighten/cover getQuayProxyTarget end-of-function fallback assumptions

  • pkg/api/promotion.go:getQuayProxyTarget already has test coverage for the “tag-based” parsing paths (e.g. ocp/ovn-kubernetes:latest, ocp/ovn-kubernetes:ci_a_latest) via TestQuayCombinedMirrorFunc / QuayCombinedMirrorFunc, so the second parsing branch’s tag.Tag + "_" prefix extraction matches the QuayTargetNameFunc-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 from target, this fallback would assume the ImageStream name and tag are both tag.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

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0b091 and b16fe83.

📒 Files selected for processing (16)
  • pkg/api/promotion.go
  • pkg/api/promotion_test.go
  • pkg/defaults/defaults.go
  • pkg/defaults/defaults_test.go
  • pkg/steps/input_image_tag_test.go
  • pkg/steps/release/create_release.go
  • pkg/steps/release/create_release_test.go
  • pkg/steps/release/promote.go
  • pkg/steps/release/promote_test.go
  • pkg/steps/release/snapshot.go
  • 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
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
  • pkg/steps/utils/image.go
  • pkg/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 deepsm007 force-pushed the revert/consolidated-quay-promotion branch from b16fe83 to 19a1202 Compare June 4, 2026 01:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/api/promotion.go (1)

117-141: 💤 Low value

Parsing 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 by TestQuayCombinedMirrorFunc. 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

📥 Commits

Reviewing files that changed from the base of the PR and between b16fe83 and 19a1202.

📒 Files selected for processing (16)
  • pkg/api/promotion.go
  • pkg/api/promotion_test.go
  • pkg/defaults/defaults.go
  • pkg/defaults/defaults_test.go
  • pkg/steps/input_image_tag_test.go
  • pkg/steps/release/create_release.go
  • pkg/steps/release/create_release_test.go
  • pkg/steps/release/promote.go
  • pkg/steps/release/promote_test.go
  • pkg/steps/release/snapshot.go
  • 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
  • pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
  • pkg/steps/utils/image.go
  • pkg/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

@Prucek

Prucek commented Jun 4, 2026

Copy link
Copy Markdown
Member

/retest-required

@deepsm007

Copy link
Copy Markdown
Contributor Author

/test images e2e

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage.

@openshift-ci

openshift-ci Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@deepsm007: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants