Repository navigation
DPTP-3787: Resolve QCI digest post-mirror via oc image info to pin spec.from in quay ImageStreams - #5123
Conversation
….from in quay ImageStreams
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
approved
WalkthroughThe changes refactor the quay promotion process to dynamically resolve image digests at runtime instead of using pre-computed tags. New helper functions parse ImageStream keys and construct shell commands for digest extraction and tag anchoring, while the promotion pod generation now handles concrete quay targets with digest-based tagging and retry logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Pod as Promotion Pod
participant Registry as Container Registry
participant IS as ImageStream
participant Shell as Shell Commands
rect rgba(100, 150, 200, 0.5)
Note over Pod,Shell: New Digest Resolution Flow
Pod->>IS: Parse concrete *-quay targets from key
IS-->>Pod: Extracted tag information
Pod->>Shell: Generate oc image info command
Shell->>Registry: Query mirrored image for digest
Registry-->>Shell: Return image manifest digest
Shell->>Pod: Extract digest via jq
Pod->>Shell: Build oc tag command with `@digest`
Shell->>IS: Apply digest-anchored tag
IS-->>Shell: Tag updated
end
rect rgba(150, 100, 150, 0.5)
Note over Pod,Shell: Prior Template-Based Flow (Fallback)
Pod->>Shell: Use pre-computed ${component} tags
Shell->>IS: Apply tag directly
IS-->>Shell: Tag applied
end
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 |
deepsm007
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml (1)
14-16: Consider validating the digest before using it.The shell commands capture the digest without validation. If
oc image inforeturns malformed JSON or the.digestfield is missing,jq -r '.digest'outputsnull, which would causeoc tagto fail with a confusing@nullpullspec error.The retry loop does catch failures, so this isn't critical, but a more robust command would provide clearer error diagnostics:
💡 Optional improvement
-_digest=$(oc image info --output=json --registry-config=/etc/push-secret/.dockerconfigjson quay-proxy.ci.openshift.org/openshift/ci:ocp_4.21_ovn-kubernetes | jq -r '.digest') && oc tag ... +_digest=$(oc image info --output=json --registry-config=/etc/push-secret/.dockerconfigjson quay-proxy.ci.openshift.org/openshift/ci:ocp_4.21_ovn-kubernetes | jq -r '.digest') && [ -n "$_digest" ] && [ "$_digest" != "null" ] && oc tag ...This would need to be applied in
getResolveAndTagCommandinpromote.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml` around lines 14 - 16, The digest extracted in getResolveAndTagCommand is used without validation, so when jq returns null (missing/malformed .digest) the subsequent oc tag gets an invalid `@null` pullspec; update getResolveAndTagCommand to validate the _digest value after running oc image info | jq -r '.digest' (check for empty/null or non-hex format), and if invalid emit a clear error/log message and force the loop to retry (or continue) instead of passing the bad value to oc tag; ensure the validated-variable name (_digest) and the retry/break logic in the existing loop are reused so failures still trigger backoff.pkg/steps/release/promote.go (2)
269-278: Add bounds check for defensive coding.
strings.LastIndex(quayProxyTag, ":")returns-1if no colon is found, which would cause a panic on the slice operationquayProxyTag[:idx]. While the current callers always pass output fromquayProxyTagFromISKey(which guarantees a colon), a defensive check would prevent future misuse.💡 Optional improvement
func getResolveAndTagCommand(registryConfig, quayProxyTag, isTag string, loglevel int) string { - repo := quayProxyTag[:strings.LastIndex(quayProxyTag, ":")] + idx := strings.LastIndex(quayProxyTag, ":") + if idx == -1 { + // Should not happen with valid quayProxyTag from quayProxyTagFromISKey + idx = len(quayProxyTag) + } + repo := quayProxyTag[:idx] return fmt.Sprintf(🤖 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 269 - 278, Add a defensive bounds check in getResolveAndTagCommand: compute idx := strings.LastIndex(quayProxyTag, ":") and if idx == -1 set repo = quayProxyTag (or otherwise handle the missing-colon case) instead of slicing directly, otherwise set repo = quayProxyTag[:idx]; then use repo in the fmt.Sprintf as before so the function cannot panic when quayProxyTag lacks a colon.
457-463: Assumption:DockerImageReferencealways contains a tag.The code uses
strings.LastIndex(ref, ":")to find the tag separator. This works correctly for refs likeregistry:5000/repo:tag(finds the tag separator) but would produce incorrect results for refs without a tag (e.g.,registry:5000/repo), where it would find the port separator instead.In practice,
DockerImageReferencefrom ImageStream status should always include a tag or digest. Consider adding a brief comment documenting this assumption for future maintainers.🤖 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 457 - 463, The logic in the block that builds a digest ref from t.Items[0].DockerImageReference assumes DockerImageReference always contains a tag (it uses strings.LastIndex(ref, ":") which can hit a registry port colon); update the code by adding a brief comment above this block noting the explicit assumption that ImageStream status DockerImageReference always includes a tag or digest (and that the LastIndex colon is intended to find the tag separator), or alternatively make the detection robust by ensuring the colon found is after the last '/' before replacing the tag; reference the variables/ref check in this snippet (t.Items[0].DockerImageReference, t.Items[0].Image, strings.LastIndex) so maintainers can locate and understand the assumption or apply the safer check.
🤖 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.go`:
- Around line 269-278: Add a defensive bounds check in getResolveAndTagCommand:
compute idx := strings.LastIndex(quayProxyTag, ":") and if idx == -1 set repo =
quayProxyTag (or otherwise handle the missing-colon case) instead of slicing
directly, otherwise set repo = quayProxyTag[:idx]; then use repo in the
fmt.Sprintf as before so the function cannot panic when quayProxyTag lacks a
colon.
- Around line 457-463: The logic in the block that builds a digest ref from
t.Items[0].DockerImageReference assumes DockerImageReference always contains a
tag (it uses strings.LastIndex(ref, ":") which can hit a registry port colon);
update the code by adding a brief comment above this block noting the explicit
assumption that ImageStream status DockerImageReference always includes a tag or
digest (and that the LastIndex colon is intended to find the tag separator), or
alternatively make the detection robust by ensuring the colon found is after the
last '/' before replacing the tag; reference the variables/ref check in this
snippet (t.Items[0].DockerImageReference, t.Items[0].Image, strings.LastIndex)
so maintainers can locate and understand the assumption or apply the safer
check.
In
`@pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml`:
- Around line 14-16: The digest extracted in getResolveAndTagCommand is used
without validation, so when jq returns null (missing/malformed .digest) the
subsequent oc tag gets an invalid `@null` pullspec; update getResolveAndTagCommand
to validate the _digest value after running oc image info | jq -r '.digest'
(check for empty/null or non-hex format), and if invalid emit a clear error/log
message and force the loop to retry (or continue) instead of passing the bad
value to oc tag; ensure the validated-variable name (_digest) and the
retry/break logic in the existing loop are reused so failures still trigger
backoff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b9f3072-d884-4cf1-b2f0-73fd5160984f
📒 Files selected for processing (3)
pkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
|
verified using https://prow.ci.openshift.org/view/gs/test-platform-results/logs/branch-ci-openshift-console-main-images/2047040221760983040 as custom ci-operator image. |
|
/test e2e |
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, jupierce 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 |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
2 similar comments
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/unhold |
4 similar comments
|
/unhold |
|
/unhold |
|
/unhold |
|
/unhold |
|
@deepsm007: The following test 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. |
be5724b
into
openshift:main
4 participants
/cc @openshift/test-platform
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests