🌱 Simplify Boxcutter applier by removing status interpretation logic

The Apply method now returns only an error instead of (bool, string, error).
The responsibility for interpreting ClusterExtensionRevision status conditions
is removed from the applier, simplifying the interface and making the applier
focus solely on creating/updating revisions.

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,7 +282,7 @@ 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) {
+func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) error {
 	return bc.apply(ctx, contentFS, ext, objectLabels, revisionAnnotations)
 }
 
@@ -308,21 +307,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 +343,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 +357,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 +366,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 +380,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)