Skip to content

Commit 767a688

Browse files
Fix SA1019: server-side apply required, needs generated apply configurations
1 parent 23dd207 commit 767a688

File tree

2 files changed

+31
-31
lines changed

2 files changed

+31
-31
lines changed

internal/operator-controller/applier/boxcutter.go

Lines changed: 12 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -439,37 +439,18 @@ func (bc *Boxcutter) createOrUpdate(ctx context.Context, user user.Info, rev *oc
439439
return err
440440
}
441441

442-
// DEPRECATION NOTICE: Using client.Apply (deprecated in controller-runtime v0.23.0+)
443-
//
444-
// WHY WE CAN'T FIX THIS YET:
445-
// The recommended replacement is the new typed Apply() method that requires generated
446-
// apply configurations (ApplyConfiguration types). However, this project does not
447-
// currently generate these apply configurations for its API types.
448-
//
449-
// WHY WE NEED SERVER-SIDE APPLY SEMANTICS:
450-
// This controller requires server-side apply with field ownership management to:
451-
// 1. Track which controller owns which fields (via client.FieldOwner)
452-
// 2. Take ownership of fields from other managers during upgrades (via client.ForceOwnership)
453-
// 3. Automatically create-or-update without explicit Get/Create/Update logic
454-
//
455-
// WHY ALTERNATIVES DON'T WORK:
456-
// - client.MergeFrom(): Lacks field ownership - causes conflicts during controller upgrades
457-
// - client.StrategicMergePatch(): No field management - upgrade tests fail with ownership errors
458-
// - Manual Create/Update: Loses server-side apply benefits, complex to implement correctly
459-
//
460-
// WHAT'S REQUIRED TO FIX PROPERLY:
461-
// 1. Generate apply configurations for all API types (ClusterExtensionRevision, etc.)
462-
// - Requires running controller-gen with --with-applyconfig flag
463-
// - Generates ClusterExtensionRevisionApplyConfiguration types
464-
// 2. Update all resource creation/update code to use typed Apply methods
465-
// 3. Update all tests to work with new patterns
466-
// This is a project-wide effort beyond the scope of the k8s v1.35 upgrade.
467-
//
468-
// MIGRATION PATH:
469-
// Track in a future issue: "Generate apply configurations and migrate to typed Apply methods"
470-
//
471-
// nolint:staticcheck // SA1019: server-side apply required, needs generated apply configurations
472-
return bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
442+
// Convert the revision to an unstructured object to create an apply configuration.
443+
// This allows us to use Server-Side Apply without needing generated apply types.
444+
unstructuredRev, err := runtime.DefaultUnstructuredConverter.ToUnstructured(rev)
445+
if err != nil {
446+
return fmt.Errorf("converting ClusterExtensionRevision to unstructured: %w", err)
447+
}
448+
u := &unstructured.Unstructured{Object: unstructuredRev}
449+
applyConfig := client.ApplyConfigurationFromUnstructured(u)
450+
451+
// Apply the revision using Server-Side Apply with field ownership to handle
452+
// field conflicts during OLM upgrades.
453+
return bc.Client.Apply(ctx, applyConfig, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
473454
}
474455

475456
func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {

internal/operator-controller/applier/boxcutter_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package applier_test
22

33
import (
44
"context"
5+
"encoding/json"
56
"errors"
67
"fmt"
78
"io"
@@ -564,6 +565,24 @@ func TestBoxcutter_Apply(t *testing.T) {
564565
}
565566
return client.Patch(ctx, obj, patch, opts...)
566567
},
568+
Apply: func(ctx context.Context, client client.WithWatch, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error {
569+
// Extract revision number from apply configuration to validate it matches expected value
570+
cer := &ocv1.ClusterExtensionRevision{}
571+
data, err := json.Marshal(obj)
572+
if err != nil {
573+
return fmt.Errorf("failed to marshal apply configuration: %w", err)
574+
}
575+
if err := json.Unmarshal(data, cer); err != nil {
576+
return fmt.Errorf("failed to unmarshal to ClusterExtensionRevision: %w", err)
577+
}
578+
579+
fmt.Println(cer.Spec.Revision)
580+
if cer.Spec.Revision != revNum {
581+
fmt.Println("AAA")
582+
return apierrors.NewInvalid(cer.GroupVersionKind().GroupKind(), cer.GetName(), field.ErrorList{field.Invalid(field.NewPath("spec.phases"), "immutable", "spec.phases is immutable")})
583+
}
584+
return client.Apply(ctx, obj, opts...)
585+
},
567586
}
568587
}
569588
testCases := []struct {

0 commit comments

Comments
 (0)