release-import optional reference_policy, payload tags, Source digest wait#5160
Open
deepsm007 wants to merge 1 commit intoopenshift:mainfrom
Open
release-import optional reference_policy, payload tags, Source digest wait#5160deepsm007 wants to merge 1 commit intoopenshift:mainfrom
deepsm007 wants to merge 1 commit intoopenshift:mainfrom
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/hold
|
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:
📝 WalkthroughWalkthroughThis PR introduces a top-level ChangesUnresolved Release Reference Policy
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
deepsm007
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
deepsm007
force-pushed
the
unresolved-release-reference-policy
branch
from
b7fbc24 to
c1b6fc7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/steps/release/import_release.go (1)
546-549: 💤 Low valueEmpty
TagReferencein spec may be ineffective.Line 548 creates an ImageStream with
Tags: []imagev1.TagReference{{ReferencePolicy: ...}}— a TagReference with noNamefield. This tag won't match any subsequent tag operations since the actualcli/latesttag is created separately viaImageStreamTag. The empty entry appears unused.Consider either removing the
Tagsfield from the spec (since the real tag is patched viaImageStreamTag) or providing the full tag definition here.💡 Suggested simplification
overrideIS := &imagev1.ImageStream{ ObjectMeta: metav1.ObjectMeta{Name: overrideCLIStreamName, Namespace: s.jobSpec.Namespace()}, - Spec: imagev1.ImageStreamSpec{LookupPolicy: imagev1.ImageLookupPolicy{Local: true}, Tags: []imagev1.TagReference{{ReferencePolicy: imagev1.TagReferencePolicy{Type: s.overrideCLIReferencePolicy()}}}}, + Spec: imagev1.ImageStreamSpec{LookupPolicy: imagev1.ImageLookupPolicy{Local: true}}, }🤖 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/import_release.go` around lines 546 - 549, The ImageStream constructed as overrideIS includes a TagReference with no Name (Tags: []imagev1.TagReference{{ReferencePolicy: ...}}) which is ineffective; either remove the Tags field from the ImageStream spec entirely (since you create/patch the tag via ImageStreamTag elsewhere) or populate the TagReference.Name (e.g. "latest" or the actual cli tag) so it matches the ImageStreamTag operations; update the overrideIS creation (and references to overrideCLIStreamName and overrideCLIReferencePolicy()) accordingly.
🤖 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/api/types.go`:
- Around line 348-353: The comment for ReferencePolicy on the struct is
inaccurate: ReferencePolicy is a single field of type
imagev1.TagReferencePolicyType serialized as reference_policy with values
"Local" or "Source", not a nested object with referencePolicy.type; update the
comment on ReferencePolicy in pkg/api/types.go to describe the actual shape
(e.g., "ReferencePolicy is serialized as reference_policy and must be either
Local or Source; when unset behavior is ..."), remove any mention of
referencePolicy.type, and ensure the wording clarifies interaction with
integration.reference_policy and inference behavior so generated docs/CRDs
reflect the real config shape.
In `@pkg/defaults/defaults.go`:
- Around line 152-155: The code only forwards
resolveConfig.Integration.ReferencePolicy into AssembleReleaseStep via
importReferencePolicy, which ignores a per-release override at
resolveConfig.ReferencePolicy; change the logic that sets importReferencePolicy
(and the analogous block around lines 166-175) to prefer
resolveConfig.ReferencePolicy when non-nil and fall back to
resolveConfig.Integration.ReferencePolicy otherwise, then pass that resulting
importReferencePolicy into AssembleReleaseStep so
releases.<name>.reference_policy is honored.
In `@pkg/util/imagestream.go`:
- Around line 42-43: The helper sourcePolicyAllowsDigestOnlyStatus currently
treats an empty policy as equivalent to Source, causing ResolvePullSpec(...,
true) to take digest-only success for tags without Source hints; change
sourcePolicyAllowsDigestOnlyStatus to return true only when policy ==
imageapi.SourceTagReferencePolicy (remove the policy == "" branch) and ensure
any logic in ResolvePullSpec that relies on digest-only behavior also verifies
the tag itself carries Source hints (e.g., reference:true or PreserveOriginal)
before accepting digest-only resolution.
---
Nitpick comments:
In `@pkg/steps/release/import_release.go`:
- Around line 546-549: The ImageStream constructed as overrideIS includes a
TagReference with no Name (Tags: []imagev1.TagReference{{ReferencePolicy: ...}})
which is ineffective; either remove the Tags field from the ImageStream spec
entirely (since you create/patch the tag via ImageStreamTag elsewhere) or
populate the TagReference.Name (e.g. "latest" or the actual cli tag) so it
matches the ImageStreamTag operations; update the overrideIS creation (and
references to overrideCLIStreamName and overrideCLIReferencePolicy())
accordingly.
🪄 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: b3fc7350-5faa-4570-ba20-b9ceef9e4c11
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (11)
pkg/api/ephemeralcluster/v1/ci.openshift.io_ephemeralclusters.yamlpkg/api/types.gopkg/defaults/defaults.gopkg/steps/release/import_release.gopkg/steps/release/import_release_policy_test.gopkg/steps/utils/image.gopkg/steps/utils/image_test.gopkg/util/imagestream.gopkg/util/imagestream_test.gopkg/validation/release.gopkg/validation/release_test.go
Prucek
reviewed
May 6, 2026
deepsm007
force-pushed
the
unresolved-release-reference-policy
branch
from
c1b6fc7 to
118b02b
Compare
Contributor
Author
|
/test e2e |
deepsm007
force-pushed
the
unresolved-release-reference-policy
branch
10 times, most recently
from
0f2f752 to
a5f58f6
Compare
Contributor
Author
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
Author
|
/test e2e |
deepsm007
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
deepsm007
force-pushed
the
unresolved-release-reference-policy
branch
from
a5f58f6 to
f5ace28
Compare
droslean
suggested changes
May 7, 2026
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/util/imagestream.go`:
- Around line 142-149: The code treats any non-empty item.DockerImageReference
as an exact pullSpec; make it defensive by checking that the chosen ref is
digest-based before assigning pullSpec: when evaluating the branch in
requireExact && digestOnlyResolutionAllowed(specTag, specRefPolicy) &&
item.Image == "" && sourceImportNotFailed(tags.Conditions), only accept
item.DockerImageReference if it contains the digest marker (e.g.
strings.Contains(item.DockerImageReference, "@sha256:")); if that check fails,
fall back to dockerDigestPullSpecFromSpecTag(is, tag) as before and only set
pullSpec when the resulting ref is digest-based. Ensure checks reference the
existing symbols item.DockerImageReference, dockerDigestPullSpecFromSpecTag,
pullSpec, requireExact, digestOnlyResolutionAllowed, specTag, specRefPolicy, and
sourceImportNotFailed.
🪄 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: a864064e-4692-4163-b349-e84ae2b1c6dd
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (14)
pkg/api/ephemeralcluster/v1/ci.openshift.io_ephemeralclusters.yamlpkg/api/reference_policy.gopkg/api/types.gopkg/defaults/defaults.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/image_resolution_test.gopkg/steps/release/import_release.gopkg/steps/release/import_release_policy_test.gopkg/steps/utils/image.gopkg/steps/utils/image_test.gopkg/util/imagestream.gopkg/util/imagestream_test.gopkg/validation/release.gopkg/validation/release_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/api/reference_policy.go
- pkg/api/types.go
- pkg/api/ephemeralcluster/v1/ci.openshift.io_ephemeralclusters.yaml
- pkg/steps/utils/image_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/validation/release_test.go
- pkg/validation/release.go
- pkg/steps/utils/image.go
- pkg/defaults/defaults.go
deepsm007
force-pushed
the
unresolved-release-reference-policy
branch
3 times, most recently
from
fb3cbbc to
b635685
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/steps/multi_stage/gen.go`:
- Around line 301-307: The imagestream lookup uses context.TODO() which can
block indefinitely; replace it with a bounded context (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), <reasonableDuration>) and defer
cancel()) when calling s.client.Get for the imagev1.ImageStream, ensure you pass
that ctx to s.client.Get (the same call that uses key and is), and add "time" to
the imports; choose a sensible timeout (e.g., 15-30s) and propagate/cancel the
context to avoid leaks.
- Around line 102-105: The current logic in gen.go lets a transient
s.resolveStepImage(stream, tag) error (resolveErr) skip the entire step by
appending resolveErr to errs and continuing; instead implement a "fail-open"
fallback: when resolveErr is non-nil, log or record the resolution error but set
the step's image to the original stream:tag (use resolvedImage = stream + ":" +
tag or the same variable used downstream) and proceed rather than appending to
errs/continuing. Update the handling around resolvedImage/resolveErr so
downstream code uses the fallback image when resolution fails, and only treat it
as fatal if other validation later indicates a real problem.
In `@pkg/steps/release/import_release.go`:
- Around line 243-250: The loop over stable.Spec.Tags is reapplying
mergedPayloadReferencePolicy and setting ImportModePreserveOriginal to tags that
were not part of the imported payload, mutating unrelated existing tags; fix by
distinguishing tags that come from the imported payload versus existing retained
tags and only apply mergedPayloadReferencePolicy (and set
t.ImportPolicy.ImportMode = imagev1.ImportModePreserveOriginal) for tags known
to be from the imported payload. Concretely, in the loop that iterates
stable.Spec.Tags (and uses existing.Has/Insert and appends to tags), check
membership against the imported-payload tag set (create/use a payloadTags set
built from the payload import) and if the tag is not in payloadTags, append the
original tag unchanged and continue; only call mergedPayloadReferencePolicy and
mutate ImportPolicy for tags present in payloadTags (and that are newly
inserted).
🪄 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: a50e2f7a-5d5d-4ee1-95f9-fb8d632da60d
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (13)
pkg/api/ephemeralcluster/v1/ci.openshift.io_ephemeralclusters.yamlpkg/api/types.gopkg/defaults/defaults.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/init_test.gopkg/steps/release/import_release.gopkg/steps/release/import_release_policy_test.gopkg/steps/utils/image.gopkg/steps/utils/image_test.gopkg/util/imagestream.gopkg/util/imagestream_test.gopkg/validation/release.gopkg/validation/release_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/api/ephemeralcluster/v1/ci.openshift.io_ephemeralclusters.yaml
- pkg/api/types.go
- pkg/steps/utils/image.go
- pkg/steps/release/import_release_policy_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/util/imagestream_test.go
- pkg/steps/utils/image_test.go
deepsm007
force-pushed
the
unresolved-release-reference-policy
branch
3 times, most recently
from
5b6627c to
87646f9
Compare
deepsm007
force-pushed
the
unresolved-release-reference-policy
branch
from
87646f9 to
fd18bcc
Compare
Contributor
Author
|
/test e2e |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
Contributor
|
@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. |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cc @Prucek @openshift/test-platform