DPTP-4340: Remove direct mirroring to registry.ci.openshift.org, consolidate to single promotion step#4866
DPTP-4340: Remove direct mirroring to registry.ci.openshift.org, consolidate to single promotion step#4866deepsm007 wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@deepsm007: This pull request references DPTP-4340 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.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. |
WalkthroughRemoved the "-quay" suffix from promotion target naming and adjusted promotion control flow: Quay promotion is now conditionally appended based on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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 |
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
when we merge this only the X.Y-quay IS will stay, is that what we want? Or do we want the current X.Y IS to use QCI images https://github.com/openshift/ci-tools/blob/main/pkg/api/promotion.go#L114 |
8c8865e to
778e4f1
Compare
|
/test images |
|
Scheduling required tests: |
4dd323b to
0c9127a
Compare
FromConfig no longer registers the legacy promotion step when promoting; update TestFromConfig expectedPost to match.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@deepsm007: This pull request references DPTP-4340 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
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 (1)
pkg/defaults/defaults_test.go (1)
1700-1705:⚠️ Potential issue | 🟡 MinorTest expectation correctly updated for consolidated promotion flow.
The removal of
"[promotion]"fromexpectedPostaligns with the code changes: the direct mirroring step is removed, leaving only"[promotion-quay]"which handles both quay.io mirroring and proxy tagging.Minor: Add test case for RegistryOverride with promotion. The current test covers the normal promote scenario, but the
RegistryOverrideconditional path (lines 307–310 in defaults.go) is not tested in the main test suite. Consider adding a test case withpromote: trueandPromotionConfiguration.RegistryOverrideset to verify no promotion steps are generated in that scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults_test.go` around lines 1700 - 1705, Add a new table test case in defaults_test.go alongside the existing promote:true case that sets promote: true and the PromotionConfiguration.RegistryOverride field to a non-empty value; name it to indicate "RegistryOverride with promotion". In that case set expectedSteps to the same pre-promotion steps (e.g., "[output-images]", "[images]") but set expectedPost to an empty slice (or assert that no promotion steps are generated) to verify the conditional path in defaults.go (the PromotionConfiguration.RegistryOverride branch around lines ~307–310) produces no promotion steps. Ensure the test populates the same input struct used by the table (referencing promote and PromotionConfiguration.RegistryOverride) and asserts equality against expectedPost and expectedSteps like the other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/defaults/defaults_test.go`:
- Around line 1700-1705: Add a new table test case in defaults_test.go alongside
the existing promote:true case that sets promote: true and the
PromotionConfiguration.RegistryOverride field to a non-empty value; name it to
indicate "RegistryOverride with promotion". In that case set expectedSteps to
the same pre-promotion steps (e.g., "[output-images]", "[images]") but set
expectedPost to an empty slice (or assert that no promotion steps are generated)
to verify the conditional path in defaults.go (the
PromotionConfiguration.RegistryOverride branch around lines ~307–310) produces
no promotion steps. Ensure the test populates the same input struct used by the
table (referencing promote and PromotionConfiguration.RegistryOverride) and
asserts equality against expectedPost and expectedSteps like the other cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d059087d-6e01-49bd-bc62-cae27a8f234a
📒 Files selected for processing (4)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.go
💤 Files with no reviewable changes (1)
- pkg/defaults/defaults.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/promotion_test.go
|
@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. |
Removes the direct mirroring step to
registry.ci.openshift.organd consolidates promotion into a single step that handles both quay.io mirroring and tagging.The promotion now:
(1) mirrors to
quay.io/openshift/ciusingoc image mirror, then(2) tags in
registry.ci.openshift.orgfromquay-proxy.ci.openshift.orgusingoc tag./hold
/cc @Prucek @openshift/test-platform
Summary by CodeRabbit
Bug Fixes
Refactor