Refactor revision controller and add unit tests"

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: Per Goncalves da Silva

cmd/operator-controller/main.go

@@ -534,9 +534,9 @@ func run() error {
 		}
 
 		if err = (&controllers.ClusterExtensionRevisionReconciler{
-			Client:        cl,
-			AccessManager: accessManager,
-			RevisionEngineGetter: controllers.OLMRevisionEngineGetter{
+			Client: cl,
+			RevisionManager: &controllers.OLMRevisionEngineGetter{
+				AccessManager:   accessManager,
 				Scheme:          mgr.GetScheme(),
 				RestMapper:      mgr.GetRESTMapper(),
 				DiscoveryClient: discoveryClient,

internal/operator-controller/applier/boxcutter_test.go

@@ -485,14 +485,10 @@ func TestBoxcutter_Apply(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			// Setup
-			fakeClientBuilder := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(tc.existingObjs...)
-			fakeClient := fakeClientBuilder.Build()
-
-			// Special handling for the create error test
-			var testClient client.Client = fakeClient
+			fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(tc.existingObjs...).Build()
 
 			boxcutter := &applier.Boxcutter{
-				Client:            testClient,
+				Client:            fakeClient,
 				Scheme:            testScheme,
 				RevisionGenerator: tc.mockBuilder,
 			}

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	types2 "pkg.package-operator.run/boxcutter/machinery/types"
 	"strings"
 	"time"
 
@@ -22,6 +21,7 @@ import (
 	"k8s.io/utils/ptr"
 	"pkg.package-operator.run/boxcutter"
 	"pkg.package-operator.run/boxcutter/machinery"
+	machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
 	"pkg.package-operator.run/boxcutter/managedcache"
 	"pkg.package-operator.run/boxcutter/ownerhandling"
 	"pkg.package-operator.run/boxcutter/validation"
@@ -38,18 +38,16 @@ import (
 )
 
 const (
-	ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner"
-
+	ClusterExtensionRevisionOwnerLabel        = "olm.operatorframework.io/owner"
 	boxcutterSystemPrefixFieldOwner           = "olm.operatorframework.io"
 	clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown"
 )
 
 // ClusterExtensionRevisionReconciler actions individual snapshots of ClusterExtensions,
 // as part of the boxcutter integration.
 type ClusterExtensionRevisionReconciler struct {
-	Client               client.Client
-	AccessManager        AccessManager
-	RevisionEngineGetter RevisionEngineGetter
+	Client          client.Client
+	RevisionManager RevisionManager
 }
 
 type AccessManager interface {
@@ -59,21 +57,28 @@ type AccessManager interface {
 }
 
 type RevisionEngine interface {
-	Teardown(ctx context.Context, rev types2.Revision, opts ...types2.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
-	Reconcile(ctx context.Context, rev types2.Revision, opts ...types2.RevisionReconcileOption) (machinery.RevisionResult, error)
+	Teardown(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
+	Reconcile(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error)
 }
 
-type RevisionEngineGetter interface {
-	GetRevisionEngineWithAccessor(accessor managedcache.Accessor) RevisionEngine
+type RevisionManager interface {
+	GetScopedRevisionEngine(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object, usedFor []client.Object) (RevisionEngine, error)
+	HandleDeletion(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object) error
+	Source(handler.EventHandler, ...predicate.Predicate) source.Source
 }
 
 type OLMRevisionEngineGetter struct {
 	DiscoveryClient *discovery.DiscoveryClient
 	Scheme          *runtime.Scheme
 	RestMapper      meta.RESTMapper
+	AccessManager   AccessManager
 }
 
-func (r *OLMRevisionEngineGetter) GetRevisionEngineWithAccessor(accessor managedcache.Accessor) RevisionEngine {
+func (r *OLMRevisionEngineGetter) GetScopedRevisionEngine(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object, usedFor []client.Object) (RevisionEngine, error) {
+	accessor, err := r.AccessManager.GetWithUser(ctx, owner, user, usedFor)
+	if err != nil {
+		return nil, fmt.Errorf("get cache: %w", err)
+	}
 	return machinery.NewRevisionEngine(
 		machinery.NewPhaseEngine(
 			machinery.NewObjectEngine(
@@ -85,7 +90,15 @@ func (r *OLMRevisionEngineGetter) GetRevisionEngineWithAccessor(accessor managed
 			validation.NewClusterPhaseValidator(r.RestMapper, accessor),
 		),
 		validation.NewRevisionValidator(), accessor,
-	)
+	), nil
+}
+
+func (r *OLMRevisionEngineGetter) HandleDeletion(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object) error {
+	return r.AccessManager.FreeWithUser(ctx, owner, user)
+}
+
+func (r *OLMRevisionEngineGetter) Source(eventHandler handler.EventHandler, p ...predicate.Predicate) source.Source {
+	return r.AccessManager.Source(eventHandler, p...)
 }
 
 //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensionrevisions,verbs=get;list;watch;update;patch;create;delete
@@ -148,12 +161,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, ce *
 		}
 	}
 
-	accessor, err := c.AccessManager.GetWithUser(ctx, ce, rev, objects)
-	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("get cache: %w", err)
-	}
-
-	re, err := c.RevisionEngineGetter.GetRevisionEngineWithAccessor(accessor)
+	re, err := c.RevisionManager.GetScopedRevisionEngine(ctx, ce, rev, objects)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -173,7 +181,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, ce *
 		if !tres.IsComplete() {
 			return ctrl.Result{}, nil
 		}
-		if err := c.AccessManager.FreeWithUser(ctx, ce, rev); err != nil {
+		if err := c.RevisionManager.HandleDeletion(ctx, ce, rev); err != nil {
 			return ctrl.Result{}, fmt.Errorf("get cache: %w", err)
 		}
 		return ctrl.Result{}, c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer)
@@ -209,7 +217,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, ce *
 		// Archive other revisions.
 		for _, a := range previous {
 			if err := c.Client.Patch(ctx, a, client.RawPatch(
-				types.MergePatchType, []byte(`{"data":{"state":"Archived"}}`))); err != nil {
+				types.MergePatchType, []byte(`{"spec":{"lifecycleState":"Archived"}}`))); err != nil {
 				return ctrl.Result{}, fmt.Errorf("archive previous Revision: %w", err)
 			}
 		}
@@ -293,7 +301,7 @@ func (c *ClusterExtensionRevisionReconciler) SetupWithManager(mgr ctrl.Manager)
 			builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}),
 		).
 		WatchesRawSource(
-			c.AccessManager.Source(
+			c.RevisionManager.Source(
 				handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &ocv1.ClusterExtensionRevision{}),
 				predicate.ResourceVersionChangedPredicate{},
 			),