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 |
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
|
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:
WalkthroughRemoved Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/hold
|
/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 |
deepsm007
force-pushed
the
DPTP-4340
branch
2 times, most recently
from
8c8865e to
778e4f1
Compare
|
/test images |
|
Scheduling required tests: |
deepsm007
force-pushed
the
DPTP-4340
branch
5 times, most recently
from
4dd323b to
0c9127a
Compare
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
deepsm007
force-pushed
the
DPTP-4340
branch
2 times, most recently
from
a88dc3b to
4c28a9a
Compare
|
@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. |
![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 (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
|
/test integration validate-prow |
deepsm007
force-pushed
the
DPTP-4340
branch
4 times, most recently
from
0ae783a to
9a5e00d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/steps/release/promote_test.go (2)
747-748: Prefer resolved destination tags in this fixture instead of${component}templates.These keys are template-form, while
getImageMirrorTargetresolves${component}beforegetPromotionPodreceives the map. Using concrete tags here would better match runtime inputs and avoid masking shell-expansion edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/promote_test.go` around lines 747 - 748, The fixture uses template keys "ci/ci:${component}" and "ci/${component}:c" which do not match how getImageMirrorTarget resolves ${component} before getPromotionPod consumes the map; update the test fixture in promote_test.go to use the concrete resolved destination tags (replace the "${component}" template strings with the actual component name values used in the test) so the map keys mirror runtime inputs expected by getImageMirrorTarget and getPromotionPod, preventing shell-expansion/template masking in the test.
1018-1018: Avoid hard-coded quay-proxy host in expected output.Line 1018 hard-codes the host while the test logic uses
api.QCIAPPCIDomain; keeping both on the same constant avoids drift.Proposed refactor
- expected: map[string]string{ - "registry.ci.openshift.org/ocp/4.22:vertical-pod-autoscaler": "quay-proxy.ci.openshift.org/openshift/ci@sha256:vpa", - }, + expected: map[string]string{ + "registry.ci.openshift.org/ocp/4.22:vertical-pod-autoscaler": fmt.Sprintf("%s/openshift/ci@sha256:vpa", api.QCIAPPCIDomain), + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/promote_test.go` at line 1018, The test hard-codes the quay-proxy host in the expected image reference; change the expected value to use the shared constant api.QCIAPPCIDomain instead of the literal "quay-proxy.ci.openshift.org". Locate the map entry in promote_test.go that currently contains "registry.ci.openshift.org/ocp/4.22:vertical-pod-autoscaler": "quay-proxy.ci.openshift.org/openshift/ci@sha256:vpa" and construct the RHS using api.QCIAPPCIDomain (e.g. build the string with fmt.Sprintf or concatenation like api.QCIAPPCIDomain + "/openshift/ci@sha256:vpa") so the test reads the host from the single source of truth.pkg/steps/release/promote.go (1)
265-266: Stabilize tag-vs-mirror routing by removing hard-coded quay-proxy host.Line 265 hard-codes
quay-proxy.ci.openshift.org; this can silently drift from the canonical domain constant and break routing behavior.Proposed refactor
- if strings.Contains(k, "${component}") || strings.Contains(imageMirrorTarget[k], "quay-proxy.ci.openshift.org") { + if strings.Contains(k, api.ComponentFormatReplacement) || strings.Contains(imageMirrorTarget[k], api.QCIAPPCIDomain) { tags = append(tags, fmt.Sprintf("%s %s", imageMirrorTarget[k], k)) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/promote.go` around lines 265 - 266, The code currently checks strings.Contains(imageMirrorTarget[k], "quay-proxy.ci.openshift.org") using a hard-coded host; replace that literal with the canonical quay-proxy domain constant used in the repo (use the existing constant instead of the string) so the condition becomes strings.Contains(imageMirrorTarget[k], <CANONICAL_QUAY_PROXY_CONSTANT>) while keeping the existing "${component}" check and appending to tags as before (refer to imageMirrorTarget, tags, and the loop variable k to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/steps/release/promote_test.go`:
- Around line 747-748: The fixture uses template keys "ci/ci:${component}" and
"ci/${component}:c" which do not match how getImageMirrorTarget resolves
${component} before getPromotionPod consumes the map; update the test fixture in
promote_test.go to use the concrete resolved destination tags (replace the
"${component}" template strings with the actual component name values used in
the test) so the map keys mirror runtime inputs expected by getImageMirrorTarget
and getPromotionPod, preventing shell-expansion/template masking in the test.
- Line 1018: The test hard-codes the quay-proxy host in the expected image
reference; change the expected value to use the shared constant
api.QCIAPPCIDomain instead of the literal "quay-proxy.ci.openshift.org". Locate
the map entry in promote_test.go that currently contains
"registry.ci.openshift.org/ocp/4.22:vertical-pod-autoscaler":
"quay-proxy.ci.openshift.org/openshift/ci@sha256:vpa" and construct the RHS
using api.QCIAPPCIDomain (e.g. build the string with fmt.Sprintf or
concatenation like api.QCIAPPCIDomain + "/openshift/ci@sha256:vpa") so the test
reads the host from the single source of truth.
In `@pkg/steps/release/promote.go`:
- Around line 265-266: The code currently checks
strings.Contains(imageMirrorTarget[k], "quay-proxy.ci.openshift.org") using a
hard-coded host; replace that literal with the canonical quay-proxy domain
constant used in the repo (use the existing constant instead of the string) so
the condition becomes strings.Contains(imageMirrorTarget[k],
<CANONICAL_QUAY_PROXY_CONSTANT>) while keeping the existing "${component}" check
and appending to tags as before (refer to imageMirrorTarget, tags, and the loop
variable k to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f9d11c5d-94ab-400e-a0c9-917e05463dfe
📒 Files selected for processing (10)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
💤 Files with no reviewable changes (1)
- pkg/defaults/defaults.go
✅ Files skipped from review due to trivial changes (2)
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
- pkg/api/promotion_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/defaults/defaults_test.go
- pkg/steps/release/create_release_test.go
- pkg/steps/release/create_release.go
- pkg/api/promotion.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/steps/release/create_release.go (1)
347-350: Consider preservingoc get imagestreamfailure context in logs.
2>/dev/nullhides the reason for falling back, which can make release assembly failures harder to diagnose. Consider logging stderr (or conditionally logging on failure) before fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/create_release.go` around lines 347 - 350, The shell invocation that runs `oc get imagestream -n %q %q -o yaml > "${RELEASE_IS_FILE}" 2>/dev/null` suppresses stderr and loses failure context; update the logic around that call (the block that writes to RELEASE_IS_FILE and the surrounding fallback branches) to capture and log stderr on failure (for example redirect stderr to a temp file or variable and call the existing logger or error handler with its contents) before executing the fallback branch (%s or %s); ensure the logged message includes the command, the imagestream name/namespace, and the captured stderr so failures are diagnosable.pkg/steps/release/promote_test.go (1)
739-765: Add one quay-promotion test with a non-digest source pullspec.Current updated cases only validate
@sha256inputs. A non-digest source case would guard the tag/mirror classification path against regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/promote_test.go` around lines 739 - 765, The test suite in promote_test.go only covers promotion inputs that include `@sha256` digests; add a new test case (e.g., name: "promotion-quay-non-digest" / stepName: "promotion-quay") alongside the existing cases to exercise a non-digest source pullspec so the tag/mirror classification path is validated. In that new case populate imageMirror with at least one mapping where the source pullspec is a tag (no `@sha256`), for example mapping "quay.io/openshift/ci:ci_a_latest" or "ci/${component}:c" to an expected mirror, and keep nodeArchitectures/namespace consistent with the other quay tests so the existing test runner (the table-driven test around these cases) will assert the classification behavior. Ensure the new case follows the same structure and assertions as "promotion-quay" / "promotion-quay-multiple-tags".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/steps/release/promote.go`:
- Around line 265-269: The routing logic misclassifies non-digest sources
because it only checks for api.QCIAPPCIDomain; change the decision to rely on
whether the target pullspec is a digest. In the block around the condition that
uses k, imageMirrorTarget, api.ComponentFormatReplacement and
api.QCIAPPCIDomain, treat an entry as a tag (append to tags) when k contains
api.ComponentFormatReplacement OR the target pullspec does NOT contain
"@sha256:"; otherwise treat it as an image to mirror (append to images). This
ensures non-digest sources are handled as tags and only true digest pullspecs
become mirror entries.
---
Nitpick comments:
In `@pkg/steps/release/create_release.go`:
- Around line 347-350: The shell invocation that runs `oc get imagestream -n %q
%q -o yaml > "${RELEASE_IS_FILE}" 2>/dev/null` suppresses stderr and loses
failure context; update the logic around that call (the block that writes to
RELEASE_IS_FILE and the surrounding fallback branches) to capture and log stderr
on failure (for example redirect stderr to a temp file or variable and call the
existing logger or error handler with its contents) before executing the
fallback branch (%s or %s); ensure the logged message includes the command, the
imagestream name/namespace, and the captured stderr so failures are diagnosable.
In `@pkg/steps/release/promote_test.go`:
- Around line 739-765: The test suite in promote_test.go only covers promotion
inputs that include `@sha256` digests; add a new test case (e.g., name:
"promotion-quay-non-digest" / stepName: "promotion-quay") alongside the existing
cases to exercise a non-digest source pullspec so the tag/mirror classification
path is validated. In that new case populate imageMirror with at least one
mapping where the source pullspec is a tag (no `@sha256`), for example mapping
"quay.io/openshift/ci:ci_a_latest" or "ci/${component}:c" to an expected mirror,
and keep nodeArchitectures/namespace consistent with the other quay tests so the
existing test runner (the table-driven test around these cases) will assert the
classification behavior. Ensure the new case follows the same structure and
assertions as "promotion-quay" / "promotion-quay-multiple-tags".
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 70813248-7bd7-4270-b32a-95cd58c21847
📒 Files selected for processing (10)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
💤 Files with no reviewable changes (1)
- pkg/defaults/defaults.go
✅ Files skipped from review due to trivial changes (1)
- pkg/defaults/defaults_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/api/promotion_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
- pkg/steps/release/create_release_test.go
FromConfig no longer registers the legacy promotion step when promoting; update TestFromConfig expectedPost to match.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/steps/release/promote.go (1)
265-273:⚠️ Potential issue | 🟠 MajorRoute host-qualified destinations to
oc image mirrorfirst.Line [269] can still classify a host-qualified destination into
oc tagwhensrcis non-digest. That can break promotion becauseoc tagexpects imagestream destinations, not full pullspec-style targets.Proposed fix
+func hasRegistryHost(ref string) bool { + first := strings.SplitN(ref, "/", 2)[0] + return strings.Contains(first, ".") || strings.Contains(first, ":") || first == "localhost" +} + func getPromotionPod(imageMirrorTarget map[string]string, timeStr string, namespace string, name string, cliVersion string, nodeArchitectures []string) *coreapi.Pod { @@ - if strings.HasPrefix(k, api.QuayOpenShiftCIRepo+":") { - // Images promoted into quay.io/openshift/ci always use oc image mirror (tag or digest sources). + if hasRegistryHost(k) { + // Host-qualified destinations must use oc image mirror. images = append(images, fmt.Sprintf("%s=%s", src, k)) } else if strings.Contains(k, api.ComponentFormatReplacement) || !strings.Contains(src, "@sha256:") || strings.Contains(src, api.QCIAPPCIDomain) { // Cluster ImageStream tags: oc tag for non-digest sources, quay-proxy imports, or ${component} templates. tags = append(tags, fmt.Sprintf("%s %s", src, k)) } else { images = append(images, fmt.Sprintf("%s=%s", src, k)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/promote.go` around lines 265 - 273, The current branch can misclassify host-qualified destinations (full pullspecs) as imagestream tags; update the logic in promote.go where imageMirrorTarget, k, src, images, and tags are handled so that any host-qualified destination (k that looks like a pullspec / contains a registry host component, e.g. contains a '/' and a domain component like '.' or ':') is routed to oc image mirror first (append to images) before considering the oc tag branch; implement this by checking the host-qualified predicate on k first and appending fmt.Sprintf("%s=%s", src, k) to images, otherwise continue with the existing ComponentFormatReplacement / digest / QCIAPPCIDomain checks to decide tags vs images.
🧹 Nitpick comments (1)
pkg/steps/release/promote_test.go (1)
769-781: Add one regression case for non-digest source + host-qualified destination.Nice addition for non-digest handling. Please also add a case where destination is host-qualified (e.g.,
registry.ci.openshift.org/...) with a tag-based source, to guard against accidentaloc tagrouting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/promote_test.go` around lines 769 - 781, Add a regression test case alongside the existing "promotion-quay-non-digest" entry to exercise a tag-based source mapped to a host-qualified destination (to prevent accidental oc tag routing). Create a new table entry with a unique name (e.g., "promotion-host-qualified-non-digest"), same stepName "promotion-quay", nodeArchitectures and namespace as needed, and an imageMirror map where a tag source like "quay.io/openshift/ci:ci_a_latest" or "ci/${component}:c" maps to a host-qualified destination such as "registry.ci.openshift.org/ci-op-xxxx/pipeline:promoted-tag" (not a `@sha256` digest). Place this new case near the existing test cases so it runs with the promote_test.go table-driven tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/steps/release/promote.go`:
- Around line 265-273: The current branch can misclassify host-qualified
destinations (full pullspecs) as imagestream tags; update the logic in
promote.go where imageMirrorTarget, k, src, images, and tags are handled so that
any host-qualified destination (k that looks like a pullspec / contains a
registry host component, e.g. contains a '/' and a domain component like '.' or
':') is routed to oc image mirror first (append to images) before considering
the oc tag branch; implement this by checking the host-qualified predicate on k
first and appending fmt.Sprintf("%s=%s", src, k) to images, otherwise continue
with the existing ComponentFormatReplacement / digest / QCIAPPCIDomain checks to
decide tags vs images.
---
Nitpick comments:
In `@pkg/steps/release/promote_test.go`:
- Around line 769-781: Add a regression test case alongside the existing
"promotion-quay-non-digest" entry to exercise a tag-based source mapped to a
host-qualified destination (to prevent accidental oc tag routing). Create a new
table entry with a unique name (e.g., "promotion-host-qualified-non-digest"),
same stepName "promotion-quay", nodeArchitectures and namespace as needed, and
an imageMirror map where a tag source like "quay.io/openshift/ci:ci_a_latest" or
"ci/${component}:c" maps to a host-qualified destination such as
"registry.ci.openshift.org/ci-op-xxxx/pipeline:promoted-tag" (not a `@sha256`
digest). Place this new case near the existing test cases so it runs with the
promote_test.go table-driven tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 438aa299-7e87-4357-87eb-a0df8990b004
📒 Files selected for processing (11)
pkg/api/promotion.gopkg/api/promotion_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/release/create_release.gopkg/steps/release/create_release_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_digest.yaml
💤 Files with no reviewable changes (1)
- pkg/defaults/defaults.go
✅ Files skipped from review due to trivial changes (4)
- pkg/api/promotion.go
- pkg/defaults/defaults_test.go
- pkg/api/promotion_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_digest.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/steps/release/create_release_test.go
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
|
/test images e2e |
| func TestBuildOcAdmReleaseNewCommand(t *testing.T) { | ||
| cfg := &api.ReleaseTagConfiguration{Name: "4.15"} | ||
| got := buildOcAdmReleaseNewCommand(cfg, "ns", "stable", "cvo", "dest:tag", "ver") | ||
| var testCases = []struct { | ||
| name string | ||
| want string | ||
| }{ | ||
| {"exports imagestream to file", `oc get imagestream -n "ns" "stable"`}, | ||
| {"captures oc get stderr", "GET_ERR"}, | ||
| {"logs stderr on get failure", "failed, stderr:"}, | ||
| {"uses from-image-stream-file", "--from-image-stream-file=${RELEASE_IS_FILE}"}, | ||
| {"file then stream with or", " || "}, | ||
| {"from-image-stream fallback", "--from-image-stream stable"}, | ||
| } | ||
| for _, testCase := range testCases { | ||
| t.Run(testCase.name, func(t *testing.T) { | ||
| if !strings.Contains(got, testCase.want) { | ||
| t.Errorf("%s: got does not contain %q:\n%s", testCase.name, testCase.want, got) | ||
| } | ||
| }) |
There was a problem hiding this comment.
I don't understand what this is testing
There was a problem hiding this comment.
it ensures the big generated shell string still has the right pieces after refactors, instead of testing behavior by running a shell in Go.
| func buildOcAdmReleaseNewCommand(config *api.ReleaseTagConfiguration, namespace, streamName, cvo, destination, version string) string { | ||
| fromFile := strings.Join(ocAdmReleaseNewArgs(config, namespace, streamName, cvo, destination, version, "${RELEASE_IS_FILE}"), " ") | ||
| fromStream := strings.Join(ocAdmReleaseNewArgs(config, namespace, streamName, cvo, destination, version, ""), " ") | ||
| return fmt.Sprintf(`( | ||
| RELEASE_IS_FILE="${ARTIFACT_DIR:-/tmp}/release-input-is.yaml" | ||
| GET_ERR="${ARTIFACT_DIR:-/tmp}/release-is-get.err" | ||
| if oc get imagestream -n %q %q -o yaml > "${RELEASE_IS_FILE}" 2>"${GET_ERR}"; then | ||
| %s || %s | ||
| else | ||
| echo "$(date -Is) oc get imagestream -n %q %q -o yaml failed, stderr:" >&2 | ||
| cat "${GET_ERR}" >&2 | ||
| %s | ||
| fi | ||
| )`, namespace, streamName, fromFile, fromStream, namespace, streamName, fromStream) |
There was a problem hiding this comment.
Could we do something with this? I don't like the logic in a shell script, but I guess not.
There was a problem hiding this comment.
Stuck with shell in the string for this design; we can still make the Go side clearer, safer, and better tested. If you want, I can do a small refactor in-repo: extract the template + add a one-shot golden expected string for the default args (no behavior change)
| src := imageMirrorTarget[k] | ||
| if strings.HasPrefix(k, api.QuayOpenShiftCIRepo+":") { | ||
| // Images promoted into quay.io/openshift/ci always use oc image mirror (tag or digest sources). | ||
| images = append(images, fmt.Sprintf("%s=%s", src, k)) | ||
| } else if strings.Contains(k, api.ComponentFormatReplacement) || !strings.Contains(src, "@sha256:") || strings.Contains(src, api.QCIAPPCIDomain) { | ||
| // Cluster ImageStream tags: oc tag for non-digest sources, quay-proxy imports, or ${component} templates. | ||
| tags = append(tags, fmt.Sprintf("%s %s", src, k)) | ||
| } else { | ||
| // Default to mirroring for quay.io targets and other registries | ||
| images = append(images, fmt.Sprintf("%s=%s", imageMirrorTarget[k], k)) | ||
| images = append(images, fmt.Sprintf("%s=%s", src, k)) |
There was a problem hiding this comment.
Could you explain this to me in an example, maybe?
There was a problem hiding this comment.
Destinations under quay.io/openshift/ci always use oc image mirror (tag or digest sources). All other destinations use oc tag when the key has ${component}, the source has no @sha256:, or the source is quay-proxy; otherwise they use oc image mirror (e.g. pipeline digest → registry.ci / similar). This fixes mis-routing that came from classifying only on quay-proxy instead of destination + digest/pull spec.
|
/retest-required |
|
/skip |
We only want 4.x imagestream and the mechanism which was used to tag from quay-proxy will be used on 4.X.Y imagestream. The $.X.Y-quay was created for testing. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, Prucek 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 |
|
New changes are detected. LGTM label has been removed. |
|
@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. |
3 participants
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.(3) Release assembly:
oc adm release newnow tries--from-image-stream-file(from oc get imagestream … -o yaml) and falls back to--from-image-streamif that fails (fromFile || fromStream when export succeeds; stream-only if oc get fails)./hold
/cc @Prucek @openshift/test-platform
Summary by CodeRabbit
Bug Fixes
Refactor
Tests