From 484448a7502b9d45f2238a6d2ce8c8d6e3bebae2 Mon Sep 17 00:00:00 2001 From: Predrag Knezevic Date: Tue, 13 Jan 2026 19:51:13 +0100 Subject: [PATCH] Simplify Boxcutter applier interface - 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 --- cmd/operator-controller/main.go | 2 +- .../operator-controller/applier/boxcutter.go | 44 +++++-------------- .../applier/boxcutter_test.go | 6 +-- .../controllers/boxcutter_reconcile_steps.go | 5 ++- .../boxcutter_reconcile_steps_apply_test.go | 13 ++---- 5 files changed, 18 insertions(+), 52 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 9da400766..2d78edda4 100644 --- a/cmd/operator-controller/main.go +++ b/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()) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 04991fe0d..63578e9cb 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/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. diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 9cc1d0a51..081747285 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/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 { diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index 0927a5f00..4c9eec36c 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/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)) diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go index 87e4490a5..63aff2b0a 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go +++ b/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)