Skip to content

Commit 17e40e1

Browse files
camilamacedo86camilamacedo86
committed
fix: rename owner label and clarify variable naming
Rename ClusterExtensionRevision owner label from `owner` to `owner-name` for consistency. Also rename misleading `storeLbls` variable to `revisionAnnotations` to clarify it contains annotations, not labels. This removes the duplicate ClusterExtensionRevisionOwnerLabel constant and uses labels.OwnerNameKey directly throughout the codebase.
1 parent 6ef62de commit 17e40e1

File tree

6 files changed

+52
-51
lines changed

6 files changed

+52
-51
lines changed

‎internal/operator-controller/applier/boxcutter.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
2727

2828
ocv1 "github.com/operator-framework/operator-controller/api/v1"
29-
"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
3029
"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
3130
)
3231

@@ -183,7 +182,7 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
183182
ObjectMeta: metav1.ObjectMeta{
184183
Annotations: annotations,
185184
Labels: map[string]string{
186-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
185+
labels.OwnerNameKey: ext.Name,
187186
},
188187
},
189188
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -217,7 +216,7 @@ type boxcutterStorageMigratorClient interface {
217216
func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.ClusterExtension, objectLabels map[string]string) error {
218217
existingRevisionList := ocv1.ClusterExtensionRevisionList{}
219218
if err := m.Client.List(ctx, &existingRevisionList, client.MatchingLabels{
220-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
219+
labels.OwnerNameKey: ext.Name,
221220
}); err != nil {
222221
return fmt.Errorf("listing ClusterExtensionRevisions before attempting migration: %w", err)
223222
}
@@ -429,7 +428,7 @@ func (bc *Boxcutter) garbageCollectOldRevisions(ctx context.Context, revisionLis
429428
func (bc *Boxcutter) getExistingRevisions(ctx context.Context, extName string) ([]ocv1.ClusterExtensionRevision, error) {
430429
existingRevisionList := &ocv1.ClusterExtensionRevisionList{}
431430
if err := bc.Client.List(ctx, existingRevisionList, client.MatchingLabels{
432-
controllers.ClusterExtensionRevisionOwnerLabel: extName,
431+
labels.OwnerNameKey: extName,
433432
}); err != nil {
434433
return nil, fmt.Errorf("listing revisions: %w", err)
435434
}

‎internal/operator-controller/applier/boxcutter_test.go‎

Lines changed: 31 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import (
2828

2929
ocv1 "github.com/operator-framework/operator-controller/api/v1"
3030
"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
31-
"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
3231
"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
3332
)
3433

@@ -86,7 +85,7 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
8685
"olm.operatorframework.io/package-name": "my-package",
8786
},
8887
Labels: map[string]string{
89-
"olm.operatorframework.io/owner": "test-123",
88+
"olm.operatorframework.io/owner-name": "test-123",
9089
},
9190
},
9291
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -178,9 +177,9 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
178177
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{})
179178
require.NoError(t, err)
180179

181-
t.Log("by checking the olm.operatorframework.io/owner label is set to the name of the ClusterExtension")
180+
t.Log("by checking the olm.operatorframework.io/owner-name label is set to the name of the ClusterExtension")
182181
require.Equal(t, map[string]string{
183-
controllers.ClusterExtensionRevisionOwnerLabel: "test-extension",
182+
labels.OwnerNameKey: "test-extension",
184183
}, rev.Labels)
185184
t.Log("by checking the revision number is 0")
186185
require.Equal(t, int64(0), rev.Spec.Revision)
@@ -344,7 +343,7 @@ func TestBoxcutter_Apply(t *testing.T) {
344343
Name: "test-ext-1",
345344
UID: "rev-uid-1",
346345
Labels: map[string]string{
347-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
346+
labels.OwnerNameKey: ext.Name,
348347
},
349348
},
350349
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -402,7 +401,7 @@ func TestBoxcutter_Apply(t *testing.T) {
402401
ObjectMeta: metav1.ObjectMeta{
403402
Annotations: revisionAnnotations,
404403
Labels: map[string]string{
405-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
404+
labels.OwnerNameKey: ext.Name,
406405
},
407406
},
408407
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -430,7 +429,7 @@ func TestBoxcutter_Apply(t *testing.T) {
430429
},
431430
validate: func(t *testing.T, c client.Client) {
432431
revList := &ocv1.ClusterExtensionRevisionList{}
433-
err := c.List(t.Context(), revList, client.MatchingLabels{controllers.ClusterExtensionRevisionOwnerLabel: ext.Name})
432+
err := c.List(t.Context(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name})
434433
require.NoError(t, err)
435434
require.Len(t, revList.Items, 1)
436435

@@ -450,7 +449,7 @@ func TestBoxcutter_Apply(t *testing.T) {
450449
ObjectMeta: metav1.ObjectMeta{
451450
Annotations: revisionAnnotations,
452451
Labels: map[string]string{
453-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
452+
labels.OwnerNameKey: ext.Name,
454453
},
455454
},
456455
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -481,7 +480,7 @@ func TestBoxcutter_Apply(t *testing.T) {
481480
},
482481
validate: func(t *testing.T, c client.Client) {
483482
revList := &ocv1.ClusterExtensionRevisionList{}
484-
err := c.List(context.Background(), revList, client.MatchingLabels{controllers.ClusterExtensionRevisionOwnerLabel: ext.Name})
483+
err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name})
485484
require.NoError(t, err)
486485
// No new revision should be created
487486
require.Len(t, revList.Items, 1)
@@ -496,7 +495,7 @@ func TestBoxcutter_Apply(t *testing.T) {
496495
ObjectMeta: metav1.ObjectMeta{
497496
Annotations: revisionAnnotations,
498497
Labels: map[string]string{
499-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
498+
labels.OwnerNameKey: ext.Name,
500499
},
501500
},
502501
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -528,7 +527,7 @@ func TestBoxcutter_Apply(t *testing.T) {
528527
},
529528
validate: func(t *testing.T, c client.Client) {
530529
revList := &ocv1.ClusterExtensionRevisionList{}
531-
err := c.List(context.Background(), revList, client.MatchingLabels{controllers.ClusterExtensionRevisionOwnerLabel: ext.Name})
530+
err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name})
532531
require.NoError(t, err)
533532
require.Len(t, revList.Items, 2)
534533

@@ -557,7 +556,7 @@ func TestBoxcutter_Apply(t *testing.T) {
557556
validate: func(t *testing.T, c client.Client) {
558557
// Ensure no revisions were created
559558
revList := &ocv1.ClusterExtensionRevisionList{}
560-
err := c.List(context.Background(), revList, client.MatchingLabels{controllers.ClusterExtensionRevisionOwnerLabel: ext.Name})
559+
err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name})
561560
require.NoError(t, err)
562561
assert.Empty(t, revList.Items)
563562
},
@@ -570,7 +569,7 @@ func TestBoxcutter_Apply(t *testing.T) {
570569
ObjectMeta: metav1.ObjectMeta{
571570
Annotations: revisionAnnotations,
572571
Labels: map[string]string{
573-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
572+
labels.OwnerNameKey: ext.Name,
574573
},
575574
},
576575
Spec: ocv1.ClusterExtensionRevisionSpec{},
@@ -582,7 +581,7 @@ func TestBoxcutter_Apply(t *testing.T) {
582581
ObjectMeta: metav1.ObjectMeta{
583582
Name: "rev-1",
584583
Labels: map[string]string{
585-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
584+
labels.OwnerNameKey: ext.Name,
586585
},
587586
},
588587
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -594,7 +593,7 @@ func TestBoxcutter_Apply(t *testing.T) {
594593
ObjectMeta: metav1.ObjectMeta{
595594
Name: "rev-2",
596595
Labels: map[string]string{
597-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
596+
labels.OwnerNameKey: ext.Name,
598597
},
599598
},
600599
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -606,7 +605,7 @@ func TestBoxcutter_Apply(t *testing.T) {
606605
ObjectMeta: metav1.ObjectMeta{
607606
Name: "rev-3",
608607
Labels: map[string]string{
609-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
608+
labels.OwnerNameKey: ext.Name,
610609
},
611610
},
612611
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -618,7 +617,7 @@ func TestBoxcutter_Apply(t *testing.T) {
618617
ObjectMeta: metav1.ObjectMeta{
619618
Name: "rev-4",
620619
Labels: map[string]string{
621-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
620+
labels.OwnerNameKey: ext.Name,
622621
},
623622
},
624623
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -630,7 +629,7 @@ func TestBoxcutter_Apply(t *testing.T) {
630629
ObjectMeta: metav1.ObjectMeta{
631630
Name: "rev-5",
632631
Labels: map[string]string{
633-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
632+
labels.OwnerNameKey: ext.Name,
634633
},
635634
},
636635
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -642,7 +641,7 @@ func TestBoxcutter_Apply(t *testing.T) {
642641
ObjectMeta: metav1.ObjectMeta{
643642
Name: "rev-6",
644643
Labels: map[string]string{
645-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
644+
labels.OwnerNameKey: ext.Name,
646645
},
647646
},
648647
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -674,7 +673,7 @@ func TestBoxcutter_Apply(t *testing.T) {
674673
ObjectMeta: metav1.ObjectMeta{
675674
Annotations: revisionAnnotations,
676675
Labels: map[string]string{
677-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
676+
labels.OwnerNameKey: ext.Name,
678677
},
679678
},
680679
Spec: ocv1.ClusterExtensionRevisionSpec{},
@@ -686,7 +685,7 @@ func TestBoxcutter_Apply(t *testing.T) {
686685
ObjectMeta: metav1.ObjectMeta{
687686
Name: "rev-1",
688687
Labels: map[string]string{
689-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
688+
labels.OwnerNameKey: ext.Name,
690689
},
691690
},
692691
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -698,7 +697,7 @@ func TestBoxcutter_Apply(t *testing.T) {
698697
ObjectMeta: metav1.ObjectMeta{
699698
Name: "rev-2",
700699
Labels: map[string]string{
701-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
700+
labels.OwnerNameKey: ext.Name,
702701
},
703702
},
704703
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -711,7 +710,7 @@ func TestBoxcutter_Apply(t *testing.T) {
711710
ObjectMeta: metav1.ObjectMeta{
712711
Name: "rev-3",
713712
Labels: map[string]string{
714-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
713+
labels.OwnerNameKey: ext.Name,
715714
},
716715
},
717716
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -723,7 +722,7 @@ func TestBoxcutter_Apply(t *testing.T) {
723722
ObjectMeta: metav1.ObjectMeta{
724723
Name: "rev-4",
725724
Labels: map[string]string{
726-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
725+
labels.OwnerNameKey: ext.Name,
727726
},
728727
},
729728
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -736,7 +735,7 @@ func TestBoxcutter_Apply(t *testing.T) {
736735
ObjectMeta: metav1.ObjectMeta{
737736
Name: "rev-5",
738737
Labels: map[string]string{
739-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
738+
labels.OwnerNameKey: ext.Name,
740739
},
741740
},
742741
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -748,7 +747,7 @@ func TestBoxcutter_Apply(t *testing.T) {
748747
ObjectMeta: metav1.ObjectMeta{
749748
Name: "rev-6",
750749
Labels: map[string]string{
751-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
750+
labels.OwnerNameKey: ext.Name,
752751
},
753752
},
754753
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -760,7 +759,7 @@ func TestBoxcutter_Apply(t *testing.T) {
760759
ObjectMeta: metav1.ObjectMeta{
761760
Name: "rev-7",
762761
Labels: map[string]string{
763-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
762+
labels.OwnerNameKey: ext.Name,
764763
},
765764
},
766765
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -794,7 +793,7 @@ func TestBoxcutter_Apply(t *testing.T) {
794793
ObjectMeta: metav1.ObjectMeta{
795794
Annotations: revisionAnnotations,
796795
Labels: map[string]string{
797-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
796+
labels.OwnerNameKey: ext.Name,
798797
},
799798
},
800799
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -830,7 +829,7 @@ func TestBoxcutter_Apply(t *testing.T) {
830829
labels.PackageNameKey: "test-package",
831830
},
832831
Labels: map[string]string{
833-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
832+
labels.OwnerNameKey: ext.Name,
834833
},
835834
},
836835
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -858,7 +857,7 @@ func TestBoxcutter_Apply(t *testing.T) {
858857
},
859858
validate: func(t *testing.T, c client.Client) {
860859
revList := &ocv1.ClusterExtensionRevisionList{}
861-
err := c.List(context.Background(), revList, client.MatchingLabels{controllers.ClusterExtensionRevisionOwnerLabel: ext.Name})
860+
err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name})
862861
require.NoError(t, err)
863862
// Should still be only 1 revision (in-place update, not new revision)
864863
require.Len(t, revList.Items, 1)
@@ -870,7 +869,7 @@ func TestBoxcutter_Apply(t *testing.T) {
870869
assert.Equal(t, "1.0.1", rev.Annotations[labels.BundleVersionKey])
871870
assert.Equal(t, "test-package", rev.Annotations[labels.PackageNameKey])
872871
// Verify owner label is still present
873-
assert.Equal(t, ext.Name, rev.Labels[controllers.ClusterExtensionRevisionOwnerLabel])
872+
assert.Equal(t, ext.Name, rev.Labels[labels.OwnerNameKey])
874873
},
875874
},
876875
}
@@ -1061,7 +1060,7 @@ func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
10611060
ObjectMeta: metav1.ObjectMeta{
10621061
Name: "test-revision",
10631062
Labels: map[string]string{
1064-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
1063+
labels.OwnerNameKey: ext.Name,
10651064
},
10661065
},
10671066
Spec: ocv1.ClusterExtensionRevisionSpec{},

‎internal/operator-controller/controllers/clusterextension_controller.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
280280
return ctrl.Result{}, err
281281
}
282282

283-
storeLbls := map[string]string{
283+
// The following values will be stored as annotations and not labels
284+
revisionAnnotations := map[string]string{
284285
labels.BundleNameKey: resolvedRevisionMetadata.Name,
285286
labels.PackageNameKey: resolvedRevisionMetadata.Package,
286287
labels.BundleVersionKey: resolvedRevisionMetadata.Version,
@@ -297,7 +298,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
297298
// to ensure exponential backoff can occur:
298299
// - Permission errors (it is not possible to watch changes to permissions.
299300
// The only way to eventually recover from permission errors is to keep retrying).
300-
rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, storeLbls)
301+
rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, revisionAnnotations)
301302

302303
// Set installed status
303304
if rolloutSucceeded {
@@ -531,7 +532,7 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e
531532
// recent revisions. We should consolidate to avoid code duplication.
532533
existingRevisionList := &ocv1.ClusterExtensionRevisionList{}
533534
if err := d.Reader.List(ctx, existingRevisionList, client.MatchingLabels{
534-
ClusterExtensionRevisionOwnerLabel: ext.Name,
535+
labels.OwnerNameKey: ext.Name,
535536
}); err != nil {
536537
return nil, fmt.Errorf("listing revisions: %w", err)
537538
}

‎internal/operator-controller/controllers/clusterextensionrevision_controller.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,10 @@ import (
3333
"sigs.k8s.io/controller-runtime/pkg/source"
3434

3535
ocv1 "github.com/operator-framework/operator-controller/api/v1"
36+
"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
3637
)
3738

3839
const (
39-
ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner"
4040
clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown"
4141
)
4242

@@ -458,15 +458,15 @@ func (c *ClusterExtensionRevisionReconciler) removeFinalizer(ctx context.Context
458458
// listPreviousRevisions returns active revisions belonging to the same ClusterExtension with lower revision numbers.
459459
// Filters out the current revision, archived revisions, deleting revisions, and revisions with equal or higher numbers.
460460
func (c *ClusterExtensionRevisionReconciler) listPreviousRevisions(ctx context.Context, rev *ocv1.ClusterExtensionRevision) ([]*ocv1.ClusterExtensionRevision, error) {
461-
ownerLabel, ok := rev.Labels[ClusterExtensionRevisionOwnerLabel]
461+
ownerLabel, ok := rev.Labels[labels.OwnerNameKey]
462462
if !ok {
463463
// No owner label means this revision isn't properly labeled - return empty list
464464
return nil, nil
465465
}
466466

467467
revList := &ocv1.ClusterExtensionRevisionList{}
468468
if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{
469-
ClusterExtensionRevisionOwnerLabel: ownerLabel,
469+
labels.OwnerNameKey: ownerLabel,
470470
}); err != nil {
471471
return nil, fmt.Errorf("listing revisions: %w", err)
472472
}
@@ -521,12 +521,12 @@ func (c *ClusterExtensionRevisionReconciler) toBoxcutterRevision(ctx context.Con
521521
for _, specObj := range specPhase.Objects {
522522
obj := specObj.Object.DeepCopy()
523523

524-
labels := obj.GetLabels()
525-
if labels == nil {
526-
labels = map[string]string{}
524+
objLabels := obj.GetLabels()
525+
if objLabels == nil {
526+
objLabels = map[string]string{}
527527
}
528-
labels[ClusterExtensionRevisionOwnerLabel] = rev.Labels[ClusterExtensionRevisionOwnerLabel]
529-
obj.SetLabels(labels)
528+
objLabels[labels.OwnerNameKey] = rev.Labels[labels.OwnerNameKey]
529+
obj.SetLabels(objLabels)
530530

531531
switch specObj.CollisionProtection {
532532
case ocv1.CollisionProtectionIfNoController, ocv1.CollisionProtectionNone:

0 commit comments

Comments
 (0)