pkg/payload: Add Images map to manifest template config#1410
Open
jhadvig wants to merge 1 commit into
Open
Conversation
Add an Images field to manifestRenderConfig, populated from the release
payload's image-references ImageStream. This allows CVO manifests in
/manifests/ to reference component images by short name using Go
template syntax: {{index .Images "component-name"}}.
Also set missingkey=zero on the template engine so that manifests
referencing template fields not yet known to an older CVO binary
render as zero values instead of causing a fatal error. This is a
forward-compatibility safety net for upgrades: when an older CVO
loads a newer payload that uses new template fields, the unknown
fields render as empty strings and the manifests are subsequently
filtered out by feature-set or capability gating.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe change adds an ChangesImage reference rendering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 NOT APPROVED This pull-request has been approved by: jhadvig The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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/payload/render.go (1)
194-210: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd regression tests for the new
Images+missingkey=zerobehavior.Line 210 changes template failure semantics, and Lines 195-205 add filtering logic. Please add focused tests for: nil
imageRef, non-DockerImagetags being ignored, and missing image keys rendering empty output. This reduces silent-template regressions.Suggested test shape
diff --git a/pkg/payload/render_test.go b/pkg/payload/render_test.go @@ func TestRenderManifest(t *testing.T) { tests := []struct { @@ + { + name: "missing image key renders empty string", + config: manifestRenderConfig{ + Images: map[string]string{"known": "quay.io/example/known:latest"}, + }, + manifestFile: "./testdata/template_with_images.yaml", + expectedManifestFile: "./testdata/template_with_images_expected_missing_key.yaml", + }, } } + +func TestImagesFromImageRef(t *testing.T) { + // cover nil imageRef, DockerImage extraction, and non-DockerImage filtering +}🤖 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/payload/render.go` around lines 194 - 210, The imagesFromImageRef function and renderManifest function have new logic that needs test coverage to prevent regressions. Add unit tests that cover: calling imagesFromImageRef with a nil imageRef parameter to verify it returns an empty map, calling imagesFromImageRef with tags that have non-DockerImage Kind values to verify they are filtered out and not included in the returned map, and calling renderManifest with template variables that reference missing image keys to verify the missingkey=zero option causes them to render as empty strings instead of causing template errors.
🤖 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/payload/render.go`:
- Around line 194-210: The imagesFromImageRef function and renderManifest
function have new logic that needs test coverage to prevent regressions. Add
unit tests that cover: calling imagesFromImageRef with a nil imageRef parameter
to verify it returns an empty map, calling imagesFromImageRef with tags that
have non-DockerImage Kind values to verify they are filtered out and not
included in the returned map, and calling renderManifest with template variables
that reference missing image keys to verify the missingkey=zero option causes
them to render as empty strings instead of causing template errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f6c9aa5e-9edb-48df-9c96-5c90eecc01a9
📒 Files selected for processing (2)
pkg/payload/payload.gopkg/payload/render.go
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
2 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.
Summary
Add
Imagesmap tomanifestRenderConfig, populated from the release payload'simage-referencesImageStream. This allows CVO manifests in/manifests/to reference component images by short name using Go template syntax:{{index .Images "component-name"}}.Also set
missingkey=zeroon the template engine so that manifests referencing template fields not yet known to an older CVO binary render as zero values instead of causing a fatal error. This is needed for upgrade safety — when an older CVO loads a newer payload that uses new template fields (like.Images), the unknown fields render as empty strings and the manifests are filtered out by feature-set or capability gating, instead of crashing the payload loading.This PR is the first of two — it adds the Go plumbing only, no new manifests. A follow-up PR will add the console plugin manifests that use
{{index.Images "cluster-update-console-plugin"}}.Related:
image-referencesentries in the releaseTrade-offs
missingkey=zeromakes template typos silent. Currently{{.ReleaseImge}}(typo) fails loudly at manifest load time. Withmissingkey=zeroit silently renders as empty string — failing at pod runtime instead of at loading time. This is mitigated byTest_cvoManifestswhich renders allinstall/manifests in CI, but it's a weaker signal than a hard error.Render()now hard-errors ifimage-referencesis missing. The bootstrap rendering path now loadsimage-referencesfrom/release-manifests/and fails if it's not there. This file is always present in a real release payload, but could break unusual test environments that don't include it./assign @wking
Summary by CodeRabbit