🐛 Add bundle-version and package-name annotations to CER phase objects#2580
🐛 Add bundle-version and package-name annotations to CER phase objects#2580pedjak wants to merge 1 commit intooperator-framework:mainfrom
Conversation
When upgrading between bundle versions that produce identical Kubernetes manifests, the installed version status was not updated because CER phases were identical across versions, causing in-place patches instead of new revision creation. Propagate bundle-version and package-name annotations onto each rendered object within CER phases so that different bundle versions always produce distinct phases, triggering new revision creation via phase immutability. As a side benefit, every applied bundle resource now carries two annotations that immediately tell an observer which package and version it belongs to. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Pull request overview
Propagates bundle metadata (package name + bundle version) onto each rendered object in ClusterExtensionRevision (CER) phases so that otherwise-identical manifests across bundle versions still produce distinct phases and trigger new revision creation (fixing installed version status not updating on upgrade).
Changes:
- Add a new test bundle version (
1.0.4) that reuses the exact same bundle image as1.0.0to reproduce the “identical manifests across versions” upgrade case. - Add an E2E update scenario that upgrades from
1.0.0to1.0.4. - Update revision generation to stamp
olm.operatorframework.io/bundle-versionandolm.operatorframework.io/package-nameas annotations on each rendered object, with unit test updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| testdata/images/catalogs/test-catalog/v1/configs/catalog.yaml | Adds a new bundle version (1.0.4) that points at the same image as 1.0.0 for regression testing identical renders across versions. |
| test/e2e/features/update.feature | Adds an E2E scenario upgrading to 1.0.4 to validate upgrade behavior when bundle content is identical. |
| internal/operator-controller/applier/boxcutter_test.go | Updates unit test expectations to include the new per-object annotations. |
| internal/operator-controller/applier/boxcutter.go | Implements propagation of bundle-version/package-name annotations onto each rendered object (Helm and non-Helm paths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| obj.SetAnnotations(mergeLabelMaps(obj.GetAnnotations(), map[string]string{ | ||
| labels.BundleVersionKey: helmRelease.Labels[labels.BundleVersionKey], | ||
| labels.PackageNameKey: helmRelease.Labels[labels.PackageNameKey], | ||
| })) |
There was a problem hiding this comment.
Minor readability: mergeLabelMaps is now used to merge both labels and annotations. Consider renaming it to something generic (e.g., mergeStringMaps) to avoid implying it’s label-specific.
| obj.SetAnnotations(mergeLabelMaps(obj.GetAnnotations(), map[string]string{ | ||
| labels.BundleVersionKey: helmRelease.Labels[labels.BundleVersionKey], | ||
| labels.PackageNameKey: helmRelease.Labels[labels.PackageNameKey], | ||
| })) |
There was a problem hiding this comment.
GenerateRevisionFromHelmRelease blindly reads helmRelease.Labels[...] and writes those values into per-object annotations. If the Helm release is missing these labels (e.g., older releases or unexpected callers), this will set empty-string annotations, which is both misleading and defeats the goal of making phases distinct across versions. Consider (a) validating the required label keys are present and returning an error if not, or (b) only merging these annotations when the values are non-empty / present.
| unstr.SetAnnotations(mergeLabelMaps(unstr.GetAnnotations(), map[string]string{ | ||
| labels.BundleVersionKey: revisionAnnotations[labels.BundleVersionKey], | ||
| labels.PackageNameKey: revisionAnnotations[labels.PackageNameKey], | ||
| })) | ||
|
|
There was a problem hiding this comment.
GenerateRevision pulls revisionAnnotations[labels.BundleVersionKey] / [labels.PackageNameKey] without checking presence, and then unconditionally writes the resulting (possibly empty) values into every rendered object annotation. This can produce resources annotated with empty bundle metadata and—if the caller ever forgets to pass these keys—won’t actually guarantee phase immutability across bundle versions. Consider requiring these keys to be set (return an error if missing) or only applying them when non-empty.
| unstr.SetAnnotations(mergeLabelMaps(unstr.GetAnnotations(), map[string]string{ | |
| labels.BundleVersionKey: revisionAnnotations[labels.BundleVersionKey], | |
| labels.PackageNameKey: revisionAnnotations[labels.PackageNameKey], | |
| })) | |
| annotationUpdates := map[string]string{} | |
| if v := revisionAnnotations[labels.BundleVersionKey]; v != "" { | |
| annotationUpdates[labels.BundleVersionKey] = v | |
| } | |
| if v := revisionAnnotations[labels.PackageNameKey]; v != "" { | |
| annotationUpdates[labels.PackageNameKey] = v | |
| } | |
| if len(annotationUpdates) > 0 { | |
| unstr.SetAnnotations(mergeLabelMaps(unstr.GetAnnotations(), annotationUpdates)) | |
| } |
| When ClusterExtension is updated to version "1.0.4" | ||
| Then ClusterExtension is rolled out | ||
| And ClusterExtension is available | ||
| And bundle "test-operator.1.0.4" is installed in version "1.0.4" |
There was a problem hiding this comment.
This e2e scenario is titled as creating a new revision, but it only asserts the installed bundle/version. To actually validate the regression (phase immutability triggers new revision creation), add assertions that the active revision name increments (e.g., ${NAME}-2) and/or that the previous revision is archived after updating to 1.0.4.
| And bundle "test-operator.1.0.4" is installed in version "1.0.4" | |
| And bundle "test-operator.1.0.4" is installed in version "1.0.4" | |
| And ClusterExtension reports "${NAME}-2" as active revision | |
| And ClusterExtensionRevision "${NAME}-2" reports Progressing as True with Reason Succeeded | |
| And ClusterExtensionRevision "${NAME}-2" reports Available as True with Reason ProbesSucceeded | |
| And ClusterExtensionRevision "${NAME}-1" is archived |
| @@ -244,6 +256,8 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { | |||
| }, | |||
| "annotations": map[string]interface{}{ | |||
| "my-annotation": "my-annotation-value", | |||
| "olm.operatorframework.io/bundle-version": "", | |||
| "olm.operatorframework.io/package-name": "", | |||
| }, | |||
There was a problem hiding this comment.
This unit test passes an empty revisionAnnotations map into GenerateRevision, which now results in per-object annotations with empty bundle-version / package-name values. Since production callers populate these annotations (and empty values are misleading), consider updating the test to pass realistic revisionAnnotations values and assert those propagated values instead of empty strings.
pedjak
requested review from
camilamacedo86,
dtfranz and
perdasilva
and removed request for
grokspawn
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2580 +/- ##
==========================================
+ Coverage 67.74% 67.80% +0.06%
==========================================
Files 137 137
Lines 9560 9568 +8
==========================================
+ Hits 6476 6488 +12
+ Misses 2585 2583 -2
+ Partials 499 497 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -75,6 +75,11 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( | |||
| _ = cache.ApplyStripAnnotationsTransform(&obj) | |||
| sanitizedUnstructured(ctx, &obj) | |||
|
|
|||
There was a problem hiding this comment.
I think you are trying to address: https://github.com/operator-framework/operator-controller/pull/2578/changes
I used the ownerRef to do so, but the PackageName and Version I think might be a better choice.
So @pedjak
Can you please just address Copilot comments?
Then, I think we are OK with this one to get merged
|
cc @grokspawn Should we be officially plumbing "release" everywhere that we plumb "version" now? |
|
I think ideally we would think in terms of CompositeVersion. I'll take a look later. |
Description
When upgrading between bundle versions that produce identical Kubernetes
manifests, the installed version status was not updated because CER phases
were identical across versions, causing in-place patches instead of new
revision creation.
Propagate bundle-version and package-name annotations onto each rendered
object within CER phases so that different bundle versions always produce
distinct phases, triggering new revision creation via phase immutability.
As a side benefit, every applied bundle resource now carries two annotations
that immediately tell an observer which package and version it belongs to.
Reviewer Checklist