Repository navigation
resolve digest-only tags in ResolvePullSpec for PreserveOriginal imports - #5172
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/hold
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo changes to image stream handling: ResolvePullSpec now returns a digest-qualified DockerImageReference when requireExact=true and the first tag item has an ChangesImage stream resolution & stable ImageStream tag merge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 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
the
approved
![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/steps/release/import_release.go (1)
237-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict forced
Localpolicy to payload-derived tags only.This now applies
Localto carried-overstable.Spec.Tagsas well, not just payload tags. That broadens behavior and can flip existing non-payload tag policy unexpectedly. Keep forcedLocalin thereleaseIS.Spec.Tagspath, and preserve policy for thestable.Spec.Tagsmerge path.Suggested patch
referencePolicy := imagev1.LocalTagReferencePolicy existing := sets.New[string]() tags := make([]imagev1.TagReference, 0, len(releaseIS.Spec.Tags)+len(stable.Spec.Tags)) for _, tag := range releaseIS.Spec.Tags { existing.Insert(tag.Name) tag.ReferencePolicy.Type = referencePolicy tag.ImportPolicy.ImportMode = imagev1.ImportModePreserveOriginal tags = append(tags, tag) } for _, tag := range stable.Spec.Tags { if existing.Has(tag.Name) { continue } existing.Insert(tag.Name) - tag.ReferencePolicy.Type = referencePolicy tag.ImportPolicy.ImportMode = imagev1.ImportModePreserveOriginal tags = append(tags, 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/steps/release/import_release.go` around lines 237 - 253, The code currently forces referencePolicy := imagev1.LocalTagReferencePolicy for both releaseIS.Spec.Tags and stable.Spec.Tags; change it so the Local override is applied only when iterating releaseIS.Spec.Tags (where you set tag.ReferencePolicy.Type = referencePolicy), and do NOT modify tag.ReferencePolicy.Type when merging stable.Spec.Tags (leave stable tags' existing ReferencePolicy intact); keep the existing ImportPolicy.ImportMode assignment if desired but remove or skip the line setting tag.ReferencePolicy.Type in the stable.Spec.Tags loop so carried-over stable tags retain their original policy.
🤖 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/steps/release/import_release.go`:
- Around line 237-253: The code currently forces referencePolicy :=
imagev1.LocalTagReferencePolicy for both releaseIS.Spec.Tags and
stable.Spec.Tags; change it so the Local override is applied only when iterating
releaseIS.Spec.Tags (where you set tag.ReferencePolicy.Type = referencePolicy),
and do NOT modify tag.ReferencePolicy.Type when merging stable.Spec.Tags (leave
stable tags' existing ReferencePolicy intact); keep the existing
ImportPolicy.ImportMode assignment if desired but remove or skip the line
setting tag.ReferencePolicy.Type in the stable.Spec.Tags loop so carried-over
stable tags retain their original policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e9a0025b-c128-4bf6-8fa4-c284d2ae3ae8
📒 Files selected for processing (1)
pkg/steps/release/import_release.go
|
Scheduling tests matching the |
7f8e76e to
4318481
Compare
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 32-36: The isLocalReferenceTag function currently requires both
st.Reference and st.ReferencePolicy.Type == imageapi.LocalTagReferencePolicy,
which incorrectly excludes tags that have the LocalTagReferencePolicy but no
Reference set; update isLocalReferenceTag to remove the st.Reference check and
return true solely when st.ReferencePolicy.Type ==
imageapi.LocalTagReferencePolicy (keeping the loop over is.Spec.Tags and tag
name match), so Local-policy tags created in import_release.go are recognized
and allow the ResolvePullSpec exact-resolution fallback to run.
🪄 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: e15637b2-f82e-4672-8d55-efaaba273a52
📒 Files selected for processing (2)
pkg/steps/release/import_release.gopkg/util/imagestream.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/steps/release/import_release.go
4318481 to
ebf83c9
Compare
deepsm007
changed the title
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 59-61: The current branch only applies the digest-qualified
fallback when tags.Items[0].Image is empty, causing exact-resolution to miss
using tags.Items[0].DockerImageReference even when Image is present; update the
logic in the function handling tag resolution (the block using
tags.Items[0].Image and tags.Items[0].DockerImageReference) so that when
requireExact is true and tags.Items[0].DockerImageReference contains "@sha256:"
you set pullSpec = tags.Items[0].DockerImageReference and exists = true
regardless of whether tags.Items[0].Image is non-empty (i.e., check the
DockerImageReference digest condition outside or in addition to the else branch
that tests Image).
🪄 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: 2a2ee24d-e59c-4a22-9b52-f82f9afd202b
📒 Files selected for processing (1)
pkg/util/imagestream.go
ebf83c9 to
74d595c
Compare
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e |
|
/test breaking-changes |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@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. |
|
/lgtm |
|
[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 |
6ebae95
into
openshift:main
2 participants
/cc @openshift/test-platform
/hold
Release Import Digest Resolution for PreserveOriginal Tags
This PR enhances the release import process to properly handle digest-only image references when using the
PreserveOriginalimport mode in the CI system's image streaming.Changes
Image Resolution Enhancement (
pkg/util/imagestream.go)The
ResolvePullSpecfunction now includes a fallback mechanism for resolving tags that contain only a digest. When an exact pull spec is required (requireExact=true) and the tag's image reference is empty, the function will return the digest-qualified reference directly from theDockerImageReferencefield (e.g.,image@sha256:abc123...). This ensures that digest-only references can be properly resolved without requiring a repository image field.Consistent Tag Reference Policy (
pkg/steps/release/import_release.go)When updating the stable ImageStream to include tags from the extracted release payload, the code now explicitly sets
Reference = falsefor all imported tags. This applies consistently to both newly imported tags from the release payload and tags preserved from the existing stable ImageStream. Combined with theImportModePreserveOriginalsetting, this ensures that original image references (including digest-only references) are preserved without being converted to local repository references.Impact
These changes improve the reliability of release imports when using
PreserveOriginalimport mode, allowing the CI system to properly handle and resolve digest-only image references that may not have traditional tag-based image identifiers. This is particularly important for release pipelines where images are referenced by their SHA256 digests rather than mutable tags.