Skip to content

🌱 Simplify Boxcutter applier interface #2446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Jan 14, 2026
+18 −52
Merged

🌱 Simplify Boxcutter applier interface #2446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
44 changes: 10 additions & 34 deletions internal/operator-controller/applier/boxcutter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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{}
Expand All @@ -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)
}
}

Expand All @@ -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
Expand All @@ -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
}
}
}
Expand All @@ -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.
Expand Down
6 changes: 1 addition & 5 deletions internal/operator-controller/applier/boxcutter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we ensure that we have the same status as before?
Is not the goal only refactory ( as described in the pr title? ) Should we change the behaviour ?

Copy link
Contributor Author

@pedjak pedjak Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already mirror statuses from active cluster revisions into ClusterExtension status. As you can see, e2e tests are passing without any change.

This PR is not just refactoring, it simplifies BoxCutter apply logic:

  • we do not try to read CER conditions and based on that report to the caller if CER installation was successful or not
  • instead, ClusterExtension reconciliation is triggered on CER update, and ClusterExtension controller is going to detect if the active revision is installed and mirror fields under .status accordingly.

}

if tc.validate != nil {
Expand Down
5 changes: 3 additions & 2 deletions internal/operator-controller/controllers/boxcutter_reconcile_steps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"cmp"
"context"
"fmt"
"io/fs"
"slices"

apimeta "k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -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{
Expand All @@ -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))
Expand Down
13 changes: 3 additions & 10 deletions internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand All @@ -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)
Expand Down
Loading