OTA-1956: pkg/payload: Add Images map and tolerate unknown template fields during upgrades#1410
Conversation
|
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:
Walkthrough
ChangesImage reference wiring and preprocessing error handling
Sequence Diagram(s)sequenceDiagram
participant LoadUpdate
participant loadPayloadTasks
participant imagesFromImageRef
participant Render
participant loadImageReferences
LoadUpdate->>LoadUpdate: Load payload & imageRef
LoadUpdate->>loadPayloadTasks: Forward payload.ImageRef
loadPayloadTasks->>imagesFromImageRef: Convert imageRef to tag→URI map
imagesFromImageRef-->>loadPayloadTasks: Return Images mapping
loadPayloadTasks->>loadPayloadTasks: Populate manifestRenderConfig.Images
Render->>loadImageReferences: Load from releaseManifestsDir
loadImageReferences-->>Render: Return ImageStream
Render->>imagesFromImageRef: Convert to tag→URI map
imagesFromImageRef-->>Render: Return Images mapping
Render->>Render: Set renderConfig.Images
Render->>Render: Execute template rendering with Images config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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
jhadvig
force-pushed
the
cvo-images-template-support
branch
from
32c326c to
6de1020
Compare
jhadvig
changed the title
jhadvig
force-pushed
the
cvo-images-template-support
branch
2 times, most recently
from
5498765 to
ddc7fc1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
jhadvig
force-pushed
the
cvo-images-template-support
branch
from
ddc7fc1 to
9579083
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/payload/payload_test.go (1)
159-172: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the test by asserting rendered content, not just filename presence.
Right now this can pass even if the valid manifest is loaded without actually rendering
{{.ReleaseImage}}. Assert the rendered payload containsimage:testfor0000_00_valid.yaml.Suggested test hardening
for _, m := range update.Manifests { if m.OriginalFilename == "0000_00_valid.yaml" { foundValid = true + if !bytes.Contains(m.Raw, []byte("release-image: 'image:test'")) && + !bytes.Contains(m.Raw, []byte(`release-image: "image:test"`)) && + !bytes.Contains(m.Raw, []byte("release-image: image:test")) { + t.Error("valid manifest should render .ReleaseImage to image:test") + } }As per path instructions, “Test Structure and Quality: ... meaningful assertion messages ...” applies here.
🤖 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/payload_test.go` around lines 159 - 172, The test currently only verifies that the valid manifest file is loaded by checking its filename, but does not validate that the template rendering actually occurred. Enhance the test by checking the rendered content of the manifest with filename "0000_00_valid.yaml" and asserting that it contains the expected rendered value "image:test" (the result of rendering the {{.ReleaseImage}} template variable). Store and inspect the rendered payload content of the valid manifest in addition to checking its OriginalFilename to ensure template substitution actually happened.Source: Path instructions
pkg/payload/render.go (2)
188-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the
Imagesfield.The new
Imagesfield inmanifestRenderConfiglacks a doc comment explaining its purpose. Adding documentation would help future maintainers understand that this field maps component short names to their resolved Docker image URIs from the release payload'simage-referencesImageStream.📝 Suggested documentation
type manifestRenderConfig struct { ReleaseImage string ClusterProfile string + // Images maps component short names to their resolved Docker image URIs + // from the release payload's image-references ImageStream. Images map[string]string }🤖 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 188 - 192, Add a doc comment above the Images field in the manifestRenderConfig struct to document its purpose and explain that it maps component short names to their resolved Docker image URIs from the release payload's image-references ImageStream. This will help future maintainers understand the field's role in the configuration structure.
195-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider defensive validation in
imagesFromImageRef.The function correctly filters for
DockerImagereferences and handles nil input, but does not validate against edge cases like:
- Empty
tag.From.Namevalues (would add empty string to map)- Duplicate tag names (second occurrence silently overwrites first)
While these cases are unlikely given ImageStream API constraints, adding defensive checks or documenting these assumptions would make the contract clearer.
🛡️ Optional defensive checks
func imagesFromImageRef(imageRef *imagev1.ImageStream) map[string]string { images := make(map[string]string) if imageRef == nil { return images } for _, tag := range imageRef.Spec.Tags { - if tag.From != nil && tag.From.Kind == "DockerImage" { + if tag.From != nil && tag.From.Kind == "DockerImage" && tag.From.Name != "" { + if _, exists := images[tag.Name]; exists { + klog.Warningf("Duplicate image tag %q in image-references, using last occurrence", tag.Name) + } images[tag.Name] = tag.From.Name } } return images }🤖 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 195 - 205, The imagesFromImageRef function does not validate that tag.From.Name is not empty before adding it to the images map, which could result in empty string keys being stored. Add a defensive check in the loop where images are being added to ensure that tag.From.Name is not an empty string before adding the tag to the map, so that only valid image references with non-empty names are stored.
🤖 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/payload_test.go`:
- Around line 159-172: The test currently only verifies that the valid manifest
file is loaded by checking its filename, but does not validate that the template
rendering actually occurred. Enhance the test by checking the rendered content
of the manifest with filename "0000_00_valid.yaml" and asserting that it
contains the expected rendered value "image:test" (the result of rendering the
{{.ReleaseImage}} template variable). Store and inspect the rendered payload
content of the valid manifest in addition to checking its OriginalFilename to
ensure template substitution actually happened.
In `@pkg/payload/render.go`:
- Around line 188-192: Add a doc comment above the Images field in the
manifestRenderConfig struct to document its purpose and explain that it maps
component short names to their resolved Docker image URIs from the release
payload's image-references ImageStream. This will help future maintainers
understand the field's role in the configuration structure.
- Around line 195-205: The imagesFromImageRef function does not validate that
tag.From.Name is not empty before adding it to the images map, which could
result in empty string keys being stored. Add a defensive check in the loop
where images are being added to ensure that tag.From.Name is not an empty string
before adding the tag to the map, so that only valid image references with
non-empty names are stored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a213a67d-c36c-4db5-8b77-d3b3fee32bc8
📒 Files selected for processing (8)
pkg/payload/payload.gopkg/payload/payload_test.gopkg/payload/render.gopkg/payload/render_test.gopkg/payload/testdata/payload-unknown-template/manifests/0000_00_valid.yamlpkg/payload/testdata/payload-unknown-template/manifests/0000_50_unknown-field.yamlpkg/payload/testdata/payload-unknown-template/release-manifests/image-referencespkg/payload/testdata/payload-unknown-template/release-manifests/release-metadata
|
/retest |
1 similar comment
|
/retest |
| } | ||
| images := imagesFromImageRef(imageRef) | ||
| if len(images) != 2 { | ||
| t.Fatalf("expected 2 images, got %d", len(images)) |
There was a problem hiding this comment.
nit: I find this kind of error message hard to debug, because... we thought we'd get two, but what did we actually get instead? Knowing the length we got is sometimes not enough to track down the divergence between what we expected to happen and where/why reality is diverging. If we're pinning length and both values, it seems like we'd have less code and easier debugging if we built up an expected map and then asserted cmp.Diff(tt.expected, actual); diff != "" as in this existing example.
There was a problem hiding this comment.
Good call, switched all three test cases to cmp.Diff with expected maps.
There was a problem hiding this comment.
Refactored to table-driven, single slice of cases with one loop calling imagesFromImageRef + cmp.Diff, matching the pattern you linked.
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
Looks good; thanks! I have one nit on test structure, but it's minor and I don't think we need to block merging on it. I'll add a hold in case you do want to address that comment pre-merge, but feel free to lift the hold without making changes, and we can always come back and tweak the test approach in a follow-up pull or if we ever fail the test-case and get pulled back into that code (which may never happen).
/hold in case you want to address feedback, fine with me if you lift this hold without addressing feedback
openshift-ci
Bot
added
do-not-merge/hold
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. |
jhadvig
force-pushed
the
cvo-images-template-support
branch
from
9579083 to
ad9ee5a
Compare
…ng upgrades
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"}}.
Make template rendering errors non-fatal during LoadUpdate. When an
older CVO binary loads a newer payload that uses template fields the
older binary does not know about (e.g. .Images), the manifest is
skipped with a warning instead of failing the entire payload load.
The new CVO binary will re-load the full payload after it replaces
the old one at run-level 0.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jhadvig
force-pushed
the
cvo-images-template-support
branch
from
ad9ee5a to
69db689
Compare
|
[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 |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@jhadvig: This PR has been marked as verified by 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. |
|
The nit I'd held for has been addressed (thanks!): /hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
I dunno what the $ w3m -dump -cols 200 'https://search.dptools.openshift.org/?maxAge=24h&type=junit&search=templateinstance+readiness+test+should+report+ready+soon+after+all+annotated+objects+are+ready' | grep 'failures match'
pull-ci-openshift-cluster-version-operator-main-e2e-agnostic-ovn (all) - 7 runs, 43% failed, 33% of failures match = 14% impact
pull-ci-openshift-origin-main-e2e-gcp-ovn (all) - 14 runs, 64% failed, 11% of failures match = 7% impact
periodic-ci-openshift-multiarch-main-nightly-4.18-ocp-e2e-aws-ovn-multi-x-ax (all) - 4 runs, 50% failed, 50% of failures match = 25% impact
periodic-ci-openshift-hypershift-release-4.19-periodics-e2e-kubevirt-metal-conformance-calico (all) - 1 runs, 100% failed, 100% of failures match = 100% impact
periodic-ci-openshift-hypershift-release-4.19-periodics-mce-e2e-kubevirt-metal-ovn-multinet-default-net (all) - 1 runs, 100% failed, 100% of failures match = 100% impact
periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-upgrade (all) - 44 runs, 55% failed, 4% of failures match = 2% impact
periodic-ci-openshift-release-main-nightly-4.23-e2e-gcp-ovn-runc (all) - 1 runs, 100% failed, 100% of failures match = 100% impact
periodic-ci-openshift-hypershift-release-4.19-periodics-mce-e2e-aws-ovn-conformance (all) - 1 runs, 100% failed, 100% of failures match = 100% impact/override ci/prow/e2e-agnostic-ovn |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-ovn 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 kubernetes-sigs/prow repository. |
|
OAuth $ w3m -dump -cols 200 'https://search.dptools.openshift.org/?maxAge=24h&type=junit&search=Head..https://test-oauth-route-e2e-test-oauth-expiration-.*EOF' | grep 'failures match'
periodic-ci-openshift-release-main-ci-4.21-upgrade-from-stable-4.20-e2e-azure-ovn-upgrade (all) - 5 runs, 20% failed, 100% of failures match = 20% impact
periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-upgrade (all) - 44 runs, 55% failed, 4% of failures match = 2% impact
periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn-upgrade (all) - 30 runs, 30% failed, 22% of failures match = 7% impact
periodic-ci-openshift-release-main-ci-4.21-upgrade-from-stable-4.20-e2e-aws-ovn-upgrade (all) - 7 runs, 14% failed, 100% of failures match = 14% impact
periodic-ci-openshift-release-main-ci-4.14-e2e-azure-ovn-upgrade (all) - 6 runs, 17% failed, 100% of failures match = 17% impact
pull-ci-openshift-api-master-e2e-aws-ovn (all) - 6 runs, 50% failed, 33% of failures match = 17% impact
pull-ci-openshift-cluster-capi-operator-main-e2e-aws-ovn (all) - 1 runs, 100% failed, 100% of failures match = 100% impact
pull-ci-openshift-cluster-monitoring-operator-main-e2e-aws-ovn-techpreview (all) - 8 runs, 88% failed, 14% of failures match = 13% impact
periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-azure-ovn-upgrade (all) - 40 runs, 23% failed, 11% of failures match = 3% impact
periodic-ci-openshift-release-main-ci-4.15-upgrade-from-stable-4.14-e2e-azure-sdn-upgrade (all) - 6 runs, 100% failed, 17% of failures match = 17% impact
pull-ci-openshift-cluster-monitoring-operator-main-e2e-aws-ovn (all) - 8 runs, 50% failed, 25% of failures match = 13% impact
periodic-ci-openshift-release-main-ci-4.15-e2e-azure-ovn-upgrade (all) - 5 runs, 40% failed, 50% of failures match = 20% impact
periodic-ci-openshift-release-main-ci-4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade (all) - 4 runs, 25% failed, 100% of failures match = 25% impact
periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade (all) - 40 runs, 18% failed, 14% of failures match = 3% impact
periodic-ci-openshift-release-main-ci-4.18-e2e-azure-ovn-upgrade (all) - 4 runs, 25% failed, 100% of failures match = 25% impact
pull-ci-openshift-origin-main-e2e-aws-ovn-fips (all) - 16 runs, 44% failed, 14% of failures match = 6% impact/override ci/prow/e2e-aws-ovn-techpreview |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-aws-ovn-techpreview 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 kubernetes-sigs/prow repository. |
|
@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. |
3 participants
Summary
Two changes to the CVO manifest template engine:
1. Add
Imagesmap tomanifestRenderConfigPopulated from the release payload's
image-referencesImageStream. CVO manifests in/manifests/can now reference component images by short name using Go template syntax:{{index .Images "component-name"}}.2. Tolerate unknown template fields during
LoadUpdateWhen an older CVO binary loads a newer payload that uses template fields the older binary does not know about (e.g.
.Images), the manifest is skipped with a warning instead of failing the entire payload load. The new CVO binary re-loads the full payload after replacing the old one at run-level 0.This is needed for upgrade safety. Without it, adding any new template field to a CVO manifest would break upgrades — the old CVO would fail to load the new payload before it can replace itself.
How it works during upgrade
Related
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Tests