Simplify Boxcutter applier interface (#2446)

- Change Apply() to return only error instead of (bool, string, error),
  removing status interpretation logic from the applier. ClusterExtensionRevision conditions
  are already mirrored to ClusterExtension.
- Change ApplyBundleWithBoxcutter to accept a function instead of an
  interface, making unit tests simpler by allowing inline function mocks

Co-authored-by: Claude <noreply@anthropic.com>

Author: pedjak

cmd/operator-controller/main.go

@@ -625,7 +625,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
 		controllers.RetrieveRevisionStates(revisionStatesGetter),
 		controllers.ResolveBundle(c.resolver),
 		controllers.UnpackBundle(c.imagePuller, c.imageCache),
-		controllers.ApplyBundleWithBoxcutter(appl),
+		controllers.ApplyBundleWithBoxcutter(appl.Apply),
 	}
 
 	baseDiscoveryClient, err := discovery.NewDiscoveryClientForConfig(c.mgr.GetConfig())

internal/operator-controller/applier/boxcutter.go

@@ -13,7 +13,6 @@ import (
 	"helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/storage/driver"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -283,10 +282,6 @@ type Boxcutter struct {
 	FieldOwner        string
 }
 
-func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
-	return bc.apply(ctx, contentFS, ext, objectLabels, revisionAnnotations)
-}
-
 func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object {
 	var objs []client.Object
 	for _, phase := range rev.Spec.Phases {
@@ -308,21 +303,21 @@ func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) erro
 	return bc.Client.Patch(ctx, obj, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
 }
 
-func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
+func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) error {
 	// Generate desired revision
 	desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations)
 	if err != nil {
-		return false, "", err
+		return err
 	}
 
 	if err := controllerutil.SetControllerReference(ext, desiredRevision, bc.Scheme); err != nil {
-		return false, "", fmt.Errorf("set ownerref: %w", err)
+		return fmt.Errorf("set ownerref: %w", err)
 	}
 
 	// List all existing revisions
 	existingRevisions, err := bc.getExistingRevisions(ctx, ext.GetName())
 	if err != nil {
-		return false, "", err
+		return err
 	}
 
 	currentRevision := &ocv1.ClusterExtensionRevision{}
@@ -344,7 +339,7 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 			// inplace patch was successful, no changes in phases
 			state = StateUnchanged
 		default:
-			return false, "", fmt.Errorf("patching %s Revision: %w", desiredRevision.Name, err)
+			return fmt.Errorf("patching %s Revision: %w", desiredRevision.Name, err)
 		}
 	}
 
@@ -358,7 +353,7 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		case StateNeedsInstall:
 			err := preflight.Install(ctx, plainObjs)
 			if err != nil {
-				return false, "", err
+				return err
 			}
 		// TODO: jlanford's IDE says that "StateNeedsUpgrade" condition is always true, but
 		//   it isn't immediately obvious why that is. Perhaps len(existingRevisions) is
@@ -367,7 +362,7 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		case StateNeedsUpgrade:
 			err := preflight.Upgrade(ctx, plainObjs)
 			if err != nil {
-				return false, "", err
+				return err
 			}
 		}
 	}
@@ -381,34 +376,15 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		desiredRevision.Spec.Revision = revisionNumber
 
 		if err = bc.garbageCollectOldRevisions(ctx, prevRevisions); err != nil {
-			return false, "", fmt.Errorf("garbage collecting old revisions: %w", err)
+			return fmt.Errorf("garbage collecting old revisions: %w", err)
 		}
 
 		if err := bc.createOrUpdate(ctx, desiredRevision); err != nil {
-			return false, "", fmt.Errorf("creating new Revision: %w", err)
+			return fmt.Errorf("creating new Revision: %w", err)
 		}
-		currentRevision = desiredRevision
 	}
 
-	progressingCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.TypeProgressing)
-	availableCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
-	succeededCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded)
-
-	if progressingCondition == nil && availableCondition == nil && succeededCondition == nil {
-		return false, "New revision created", nil
-	} else if progressingCondition != nil && progressingCondition.Status == metav1.ConditionTrue {
-		switch progressingCondition.Reason {
-		case ocv1.ReasonSucceeded:
-			return true, "", nil
-		case ocv1.ClusterExtensionRevisionReasonRetrying:
-			return false, "", errors.New(progressingCondition.Message)
-		default:
-			return false, progressingCondition.Message, nil
-		}
-	} else if succeededCondition != nil && succeededCondition.Status != metav1.ConditionTrue {
-		return false, succeededCondition.Message, nil
-	}
-	return true, "", nil
+	return nil
 }
 
 // garbageCollectOldRevisions deletes archived revisions beyond ClusterExtensionRevisionRetentionLimit.

internal/operator-controller/applier/boxcutter_test.go

@@ -926,18 +926,14 @@ func TestBoxcutter_Apply(t *testing.T) {
 					labels.PackageNameKey:   "test-package",
 				}
 			}
-			installSucceeded, installStatus, err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionAnnotations)
+			err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionAnnotations)
 
 			// Assert
 			if tc.expectedErr != "" {
 				require.Error(t, err)
 				assert.Contains(t, err.Error(), tc.expectedErr)
-				assert.False(t, installSucceeded)
-				assert.Empty(t, installStatus)
 			} else {
 				require.NoError(t, err)
-				assert.False(t, installSucceeded)
-				assert.Equal(t, "New revision created", installStatus)
 			}
 
 			if tc.validate != nil {

internal/operator-controller/controllers/boxcutter_reconcile_steps.go

@@ -20,6 +20,7 @@ import (
 	"cmp"
 	"context"
 	"fmt"
+	"io/fs"
 	"slices"
 
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
@@ -93,7 +94,7 @@ func MigrateStorage(m StorageMigrator) ReconcileStepFunc {
 	}
 }
 
-func ApplyBundleWithBoxcutter(a Applier) ReconcileStepFunc {
+func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) error) ReconcileStepFunc {
 	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
 		revisionAnnotations := map[string]string{
@@ -108,7 +109,7 @@ func ApplyBundleWithBoxcutter(a Applier) ReconcileStepFunc {
 		}
 
 		l.Info("applying bundle contents")
-		if _, _, err := a.Apply(ctx, state.imageFS, ext, objLbls, revisionAnnotations); err != nil {
+		if err := apply(ctx, state.imageFS, ext, objLbls, revisionAnnotations); err != nil {
 			// If there was an error applying the resolved bundle,
 			// report the error via the Progressing condition.
 			setStatusProgressing(ext, wrapErrorWithResolutionInfo(state.resolvedRevisionMetadata.BundleMetadata, err))

internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go

@@ -28,14 +28,6 @@ import (
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 )
 
-type mockApplier struct {
-	err error
-}
-
-func (m *mockApplier) Apply(_ context.Context, _ fs.FS, _ *ocv1.ClusterExtension, _ map[string]string, _ map[string]string) (bool, string, error) {
-	return true, "", m.err
-}
-
 func TestApplyBundleWithBoxcutter(t *testing.T) {
 	type args struct {
 		activeRevisions []ocv1.RevisionStatus
@@ -119,7 +111,6 @@ func TestApplyBundleWithBoxcutter(t *testing.T) {
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			ctx := context.Background()
-			applier := &mockApplier{}
 
 			ext := &ocv1.ClusterExtension{
 				ObjectMeta: metav1.ObjectMeta{
@@ -142,7 +133,9 @@ func TestApplyBundleWithBoxcutter(t *testing.T) {
 				imageFS: fstest.MapFS{},
 			}
 
-			stepFunc := ApplyBundleWithBoxcutter(applier)
+			stepFunc := ApplyBundleWithBoxcutter(func(_ context.Context, _ fs.FS, _ *ocv1.ClusterExtension, _, _ map[string]string) error {
+				return nil
+			})
 			result, err := stepFunc(ctx, state, ext)
 			require.NoError(t, err)
 			require.Nil(t, result)