(fix):ClusterExtensionRevision had inconsistent owner labeling and confusing code.

Author: camilamacedo86

internal/operator-controller/applier/boxcutter.go

@@ -183,7 +183,7 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: annotations,
 			Labels: map[string]string{
-				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+				labels.OwnerNameKey: ext.Name,
 			},
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{

internal/operator-controller/applier/boxcutter_test.go

@@ -86,7 +86,7 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
 				"olm.operatorframework.io/package-name":     "my-package",
 			},
 			Labels: map[string]string{
-				"olm.operatorframework.io/owner": "test-123",
+				"olm.operatorframework.io/owner-name": "test-123",
 			},
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -178,9 +178,9 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
 	rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{})
 	require.NoError(t, err)
 
-	t.Log("by checking the olm.operatorframework.io/owner label is set to the name of the ClusterExtension")
+	t.Log("by checking the owner label is set correctly (owner-name)")
 	require.Equal(t, map[string]string{
-		controllers.ClusterExtensionRevisionOwnerLabel: "test-extension",
+		labels.OwnerNameKey: "test-extension",
 	}, rev.Labels)
 	t.Log("by checking the revision number is 0")
 	require.Equal(t, int64(0), rev.Spec.Revision)
@@ -288,13 +288,13 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
 		ManifestProvider: r,
 	}
 
-	revAnnotations := map[string]string{
+	revisionAnnotations := map[string]string{
 		"other": "value",
 	}
 
 	rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{
 		"some": "value",
-	}, revAnnotations)
+	}, revisionAnnotations)
 	require.NoError(t, err)
 	t.Log("by checking the rendered objects contain the given object labels")
 	for _, phase := range rev.Spec.Phases {
@@ -305,8 +305,8 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
 			}, revObj.Object.GetLabels())
 		}
 	}
-	t.Log("by checking the generated revision contain the given annotations")
-	require.Equal(t, revAnnotations, rev.Annotations)
+	t.Log("by checking the generated revision contains the given annotations")
+	require.Equal(t, revisionAnnotations, rev.Annotations)
 }
 
 func Test_SimpleRevisionGenerator_Failure(t *testing.T) {
@@ -344,7 +344,7 @@ func TestBoxcutter_Apply(t *testing.T) {
 			Name: "test-ext-1",
 			UID:  "rev-uid-1",
 			Labels: map[string]string{
-				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+				labels.OwnerNameKey: ext.Name,
 			},
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -400,10 +400,10 @@ func TestBoxcutter_Apply(t *testing.T) {
 				makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
 					return &ocv1.ClusterExtensionRevision{
 						ObjectMeta: metav1.ObjectMeta{
-							Annotations: revisionAnnotations,
 							Labels: map[string]string{
-								controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+								labels.OwnerNameKey: ext.Name,
 							},
+							Annotations: revisionAnnotations,
 						},
 						Spec: ocv1.ClusterExtensionRevisionSpec{
 							Phases: []ocv1.ClusterExtensionRevisionPhase{
@@ -787,15 +787,15 @@ func TestBoxcutter_Apply(t *testing.T) {
 			},
 		},
 		{
-			name: "annotation-only update (same phases, different annotations)",
+			name: "annotation update (same phases, different metadata)",
 			mockBuilder: &mockBundleRevisionBuilder{
 				makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
 					return &ocv1.ClusterExtensionRevision{
 						ObjectMeta: metav1.ObjectMeta{
-							Annotations: revisionAnnotations,
 							Labels: map[string]string{
-								controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+								labels.OwnerNameKey: ext.Name,
 							},
+							Annotations: revisionAnnotations,
 						},
 						Spec: ocv1.ClusterExtensionRevisionSpec{
 							Phases: []ocv1.ClusterExtensionRevisionPhase{
@@ -830,7 +830,7 @@ func TestBoxcutter_Apply(t *testing.T) {
 							labels.PackageNameKey:   "test-package",
 						},
 						Labels: map[string]string{
-							controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+							labels.OwnerNameKey: ext.Name,
 						},
 					},
 					Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -870,7 +870,7 @@ func TestBoxcutter_Apply(t *testing.T) {
 				assert.Equal(t, "1.0.1", rev.Annotations[labels.BundleVersionKey])
 				assert.Equal(t, "test-package", rev.Annotations[labels.PackageNameKey])
 				// Verify owner label is still present
-				assert.Equal(t, ext.Name, rev.Labels[controllers.ClusterExtensionRevisionOwnerLabel])
+				assert.Equal(t, ext.Name, rev.Labels[labels.OwnerNameKey])
 			},
 		},
 	}
@@ -896,8 +896,8 @@ func TestBoxcutter_Apply(t *testing.T) {
 
 			// Execute
 			revisionAnnotations := map[string]string{}
-			if tc.name == "annotation-only update (same phases, different annotations)" {
-				// For annotation-only update test, pass NEW annotations
+			if tc.name == "annotation update (same phases, different metadata)" {
+				// For annotation update test, pass NEW metadata in annotations
 				revisionAnnotations = map[string]string{
 					labels.BundleVersionKey: "1.0.1",
 					labels.PackageNameKey:   "test-package",
@@ -1045,7 +1045,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 
 // mockBundleRevisionBuilder is a mock implementation of the ClusterExtensionRevisionGenerator for testing.
 type mockBundleRevisionBuilder struct {
-	makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error)
+	makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error)
 }
 
 func (m *mockBundleRevisionBuilder) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
@@ -1061,7 +1061,7 @@ func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "test-revision",
 			Labels: map[string]string{
-				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+				labels.OwnerNameKey: ext.Name,
 			},
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{},

internal/operator-controller/applier/helm.go

@@ -102,7 +102,7 @@ func (h *Helm) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterE
 	return nil
 }
 
-func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) (bool, string, error) {
+func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, storageLabels map[string]string) (bool, string, error) {
 	chrt, err := h.buildHelmChart(contentFS, ext)
 	if err != nil {
 		return false, "", err

internal/operator-controller/controllers/clusterextension_controller.go

@@ -280,7 +280,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
 		return ctrl.Result{}, err
 	}
 
-	storeLbls := map[string]string{
+	revisionAnnotations := map[string]string{
 		labels.BundleNameKey:      resolvedRevisionMetadata.Name,
 		labels.PackageNameKey:     resolvedRevisionMetadata.Package,
 		labels.BundleVersionKey:   resolvedRevisionMetadata.Version,
@@ -297,7 +297,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
 	// to ensure exponential backoff can occur:
 	//   - Permission errors (it is not possible to watch changes to permissions.
 	//     The only way to eventually recover from permission errors is to keep retrying).
-	rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, storeLbls)
+	rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, revisionAnnotations)
 
 	// Set installed status
 	if rolloutSucceeded {
@@ -549,9 +549,7 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e
 			continue
 		}
 
-		// TODO: the setting of these annotations (happens in boxcutter applier when we pass in "revisionAnnotations")
-		//   is fairly decoupled from this code where we get the annotations back out. We may want to co-locate
-		//   the set/get logic a bit better to make it more maintainable and less likely to get out of sync.
+		// Read bundle metadata from annotations (informational, not queried)
 		rm := &RevisionMetadata{
 			Package: rev.Annotations[labels.PackageNameKey],
 			Image:   rev.Annotations[labels.BundleReferenceKey],

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -36,7 +36,7 @@ import (
 )
 
 const (
-	ClusterExtensionRevisionOwnerLabel        = "olm.operatorframework.io/owner"
+	ClusterExtensionRevisionOwnerLabel        = "olm.operatorframework.io/owner-name"
 	clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown"
 )