Skip to content

Commit b18a28b

Browse files
pedjakclaude
andcommitted
Address PR review feedback for CER phase annotations
- Rename mergeLabelMaps to mergeStringMaps since it handles both labels and annotations - Only set bundle-version/package-name annotations when values are non-empty - Update unit test to use realistic revisionAnnotations values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 8eb6722 commit b18a28b

2 files changed

Lines changed: 35 additions & 20 deletions

File tree

internal/operator-controller/applier/boxcutter.go

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -68,17 +68,23 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
6868
if err := yaml.Unmarshal([]byte(doc), &obj); err != nil {
6969
return nil, err
7070
}
71-
obj.SetLabels(mergeLabelMaps(obj.GetLabels(), objectLabels))
71+
obj.SetLabels(mergeStringMaps(obj.GetLabels(), objectLabels))
7272

7373
// Memory optimization: strip large annotations
7474
// Note: ApplyStripTransform never returns an error in practice
7575
_ = cache.ApplyStripAnnotationsTransform(&obj)
7676
sanitizedUnstructured(ctx, &obj)
7777

78-
obj.SetAnnotations(mergeLabelMaps(obj.GetAnnotations(), map[string]string{
79-
labels.BundleVersionKey: helmRelease.Labels[labels.BundleVersionKey],
80-
labels.PackageNameKey: helmRelease.Labels[labels.PackageNameKey],
81-
}))
78+
annotationUpdates := map[string]string{}
79+
if v := helmRelease.Labels[labels.BundleVersionKey]; v != "" {
80+
annotationUpdates[labels.BundleVersionKey] = v
81+
}
82+
if v := helmRelease.Labels[labels.PackageNameKey]; v != "" {
83+
annotationUpdates[labels.PackageNameKey] = v
84+
}
85+
if len(annotationUpdates) > 0 {
86+
obj.SetAnnotations(mergeStringMaps(obj.GetAnnotations(), annotationUpdates))
87+
}
8288

8389
objs = append(objs, *ocv1ac.ClusterExtensionRevisionObject().
8490
WithObject(obj))
@@ -131,7 +137,7 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
131137
// objectLabels
132138
objs := make([]ocv1ac.ClusterExtensionRevisionObjectApplyConfiguration, 0, len(plain))
133139
for _, obj := range plain {
134-
obj.SetLabels(mergeLabelMaps(obj.GetLabels(), objectLabels))
140+
obj.SetLabels(mergeStringMaps(obj.GetLabels(), objectLabels))
135141

136142
gvk, err := apiutil.GVKForObject(obj, r.Scheme)
137143
if err != nil {
@@ -151,10 +157,16 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
151157
}
152158
sanitizedUnstructured(ctx, &unstr)
153159

154-
unstr.SetAnnotations(mergeLabelMaps(unstr.GetAnnotations(), map[string]string{
155-
labels.BundleVersionKey: revisionAnnotations[labels.BundleVersionKey],
156-
labels.PackageNameKey: revisionAnnotations[labels.PackageNameKey],
157-
}))
160+
annotationUpdates := map[string]string{}
161+
if v := revisionAnnotations[labels.BundleVersionKey]; v != "" {
162+
annotationUpdates[labels.BundleVersionKey] = v
163+
}
164+
if v := revisionAnnotations[labels.PackageNameKey]; v != "" {
165+
annotationUpdates[labels.PackageNameKey] = v
166+
}
167+
if len(annotationUpdates) > 0 {
168+
unstr.SetAnnotations(mergeStringMaps(unstr.GetAnnotations(), annotationUpdates))
169+
}
158170

159171
objs = append(objs, *ocv1ac.ClusterExtensionRevisionObject().
160172
WithObject(unstr))
@@ -681,9 +693,9 @@ func revisionManagementPerms(rev *ocv1ac.ClusterExtensionRevisionApplyConfigurat
681693
}
682694
}
683695

684-
func mergeLabelMaps(m1, m2 map[string]string) map[string]string {
685-
mergedLabels := make(map[string]string, len(m1)+len(m2))
686-
maps.Copy(mergedLabels, m1)
687-
maps.Copy(mergedLabels, m2)
688-
return mergedLabels
696+
func mergeStringMaps(m1, m2 map[string]string) map[string]string {
697+
merged := make(map[string]string, len(m1)+len(m2))
698+
maps.Copy(merged, m1)
699+
maps.Copy(merged, m2)
700+
return merged
689701
}

internal/operator-controller/applier/boxcutter_test.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,10 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
207207
},
208208
}
209209

210-
rev, err := b.GenerateRevision(t.Context(), dummyBundle, ext, map[string]string{}, map[string]string{})
210+
rev, err := b.GenerateRevision(t.Context(), dummyBundle, ext, map[string]string{}, map[string]string{
211+
labels.BundleVersionKey: "1.0.0",
212+
labels.PackageNameKey: "test-package",
213+
})
211214
require.NoError(t, err)
212215

213216
t.Log("by checking the olm.operatorframework.io/owner-name and owner-kind labels are set")
@@ -232,8 +235,8 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
232235
"metadata": map[string]interface{}{
233236
"name": "test-service",
234237
"annotations": map[string]interface{}{
235-
"olm.operatorframework.io/bundle-version": "",
236-
"olm.operatorframework.io/package-name": "",
238+
"olm.operatorframework.io/bundle-version": "1.0.0",
239+
"olm.operatorframework.io/package-name": "test-package",
237240
},
238241
},
239242
"spec": map[string]interface{}{},
@@ -256,8 +259,8 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
256259
},
257260
"annotations": map[string]interface{}{
258261
"my-annotation": "my-annotation-value",
259-
"olm.operatorframework.io/bundle-version": "",
260-
"olm.operatorframework.io/package-name": "",
262+
"olm.operatorframework.io/bundle-version": "1.0.0",
263+
"olm.operatorframework.io/package-name": "test-package",
261264
},
262265
},
263266
"spec": map[string]interface{}{

0 commit comments

Comments
 (0)