OTA-1956: oc adm release new: Include base image's image-references in pruning#2287
OTA-1956: oc adm release new: Include base image's image-references in pruning#2287jhadvig wants to merge 2 commits into
Conversation
The CVO image is the release base image but lacks the io.openshift.release.operator=true label, so its manifests are not extracted during release building. This means any images declared in the CVO's install/image-references (like cluster-update-console-plugin) are not seen by pruneUnreferencedImageStreams and get dropped from the release payload. When the base image has no operator label, still extract its manifests so the image-references file is available during pruning. Exclude the base image from the ordered list so its manifests are not duplicated into release-manifests/ — they already live in /manifests/ from the base layer.
WalkthroughThe PR removes the configured base image tag from the release ordered list and adds a manifest extraction special-case that allows creating the destination directory and extracting the base tag's image-references independently of operator annotations. Test helpers and a new test verify pruning of unreferenced ImageStream tags across operator/base-image scenarios. ChangesBase image tag handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jhadvig
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references OTA-1956 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. |
Local validationTested the fix locally using SetupCreated a minimal release directory with a With the fix — CVO's image-references prevents pruning$ /tmp/oc-custom adm release new --from-dir=/tmp/release-test --to-file=/tmp/test-release.tar
info: Using /tmp/release-test as the input to the release
info: Found 1 operator manifest directories on disk
info: Included 2 images from 1 input operators into the releaseOutput release contains both Without CVO's image-references — image is pruned$ rm /tmp/release-test/cluster-version-operator/image-references
$ /tmp/oc-custom adm release new --from-dir=/tmp/release-test --to-file=/tmp/test-release-no-ref.tar
info: Using /tmp/release-test as the input to the release
info: Found 1 operator manifest directories on disk
info: Included 1 images from 1 input operators into the releaseOutput release only contains |
| t.Errorf("expected %s to be removed from ordered", baseTag) | ||
| } | ||
| if len(ordered) != 2 { | ||
| t.Errorf("expected 2 entries, got %d", len(ordered)) |
There was a problem hiding this comment.
is this just excercising the slices library? I'm ok trusting the stdlib to do what it says it will do, and not building our own custom tests that just excercise the stdlib. Maybe we can drop this? Or maybe you want to bump up a level and excercise some of the new.go logic you're adding that uses those slices functions?
There was a problem hiding this comment.
Good point, dropped it 🎤
| for _, name := range names { | ||
| is.Spec.Tags = append(is.Spec.Tags, imageapi.TagReference{ | ||
| Name: name, | ||
| From: &corev1.ObjectReference{Kind: "DockerImage", Name: "example.com/" + name + ":latest"}, |
There was a problem hiding this comment.
Overlaps a bit with writeImageReferences. Maybe adjust writeImageReferences to return the ImageStream structure it wrote to disk, if we need an in-memory copy? Or refactor to pull the ImageStream structure creation out into a createImageStream helper that both the tests and writeImageReferences can consume?
There was a problem hiding this comment.
Refactored... extracted a shared createImageStream helper that both writeImageReferences and the test cases use.
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
I left two nits with the test suite, but I don't think either needs to block the merge. I'm approving and holding, in case you want to address either. But feel free to lift the hold whenever you like, whether or not you make any changes.
/llgtm
/hold in case you want to address test-case feedback
openshift-ci
Bot
added
do-not-merge/hold
- Drop TestBaseImageExcludedFromOrdered (just exercised stdlib slices) - Extract createImageStream helper shared by writeImageReferences and tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cli/admin/release/new_test.go (1)
62-150: ⚡ Quick winConvert this into a table-driven test and compare the final tag set in one assertion.
These three subtests are the same arrange/act/assert shape with different inputs and expected tags. Folding them into a table and asserting
tagNames(is)with a singlecmp.Diffwould match the repo’s test conventions and make future pruning cases easier to extend.As per coding guidelines, "Write unit tests for every change using table-driven test patterns and standard
testing.T" and "Usegoogle/go-cmpto compare expected and actual objects rather than checking individual fields with if statements."🤖 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/cli/admin/release/new_test.go` around lines 62 - 150, The test TestPruneUnreferencedImageStreams should be converted to a table-driven test: replace the three near-duplicate t.Run blocks with a single slice of test cases (fields: name, setup inputs such as operator/base image references created via writeImageReferences and createImageStream, metadata map, include list, and expected tag slice), loop over cases calling pruneUnreferencedImageStreams, then compute actual := tagNames(is) and assert equality with cmp.Diff in one assertion per case (use t.Fatalf on prune error and t.Errorf with the diff on mismatch). Keep references to the existing helpers pruneUnreferencedImageStreams, tagNames, writeImageReferences, and createImageStream so the setup/act/assert logic is identical but consolidated into the table-driven structure.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/cli/admin/release/new_test.go`:
- Around line 62-150: The test TestPruneUnreferencedImageStreams should be
converted to a table-driven test: replace the three near-duplicate t.Run blocks
with a single slice of test cases (fields: name, setup inputs such as
operator/base image references created via writeImageReferences and
createImageStream, metadata map, include list, and expected tag slice), loop
over cases calling pruneUnreferencedImageStreams, then compute actual :=
tagNames(is) and assert equality with cmp.Diff in one assertion per case (use
t.Fatalf on prune error and t.Errorf with the diff on mismatch). Keep references
to the existing helpers pruneUnreferencedImageStreams, tagNames,
writeImageReferences, and createImageStream so the setup/act/assert logic is
identical but consolidated into the table-driven structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 00971d67-9a3a-4511-bc11-2f8fab30fc0f
📒 Files selected for processing (1)
pkg/cli/admin/release/new_test.go
|
@jhadvig: all tests passed! 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, wking 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 |
4 participants
Summary
The CVO image is the release base image (
ToImageBaseTag: "cluster-version-operator") but does not carry theio.openshift.release.operator=truelabel. This meansextractManifestsskips it entirely, and any images declared in the CVO'sinstall/image-referencesare invisible topruneUnreferencedImageStreams— they get dropped from the release payload.This blocks openshift/cluster-version-operator#1398, which adds a
cluster-update-console-plugindeployment to the CVO's own manifests. The plugin image exists in the CI ImageStream but is pruned from the release because no operator'simage-referencesdeclares it.Root cause
extractManifestsiterates all tags and checks forio.openshift.release.operator=true(line 1037). CVO has no such label → extraction is skipped → no files in the temp directory.pruneUnreferencedImageStreams(line 1591) scans each extracted operator's directory forimage-references. Since CVO was never extracted, itsimage-referencesis never read.cluster-update-console-pluginis not referenced by any operator and not inAlwaysInclude→ it is pruned from the release.Implementation
Two small changes in
pkg/cli/admin/release/new.go:extractManifestsConditionFn: When an image has no operator label but IS theToImageBaseTag, still create the destination directory and proceed with extraction. This makes the CVO'simage-referencesfile available on disk for the pruning step.Exclude base image from
ordered: After the metadata-based filter, remove the base image tag fromordered. Its manifests already live in/manifests/from the base layer and must not be duplicated intorelease-manifests/.Tests
Added
TestPruneUnreferencedImageStreams(3 cases):image-referenceskeeps referenced images (existing behavior)image-referencesprevents pruning (new behavior)image-references, the image is pruned (the bug)Added
TestBaseImageExcludedFromOrdered(2 cases):orderedRelated
/assign @wking @DavidHurta
Summary by CodeRabbit
Bug Fixes
Tests