Make ClusterExtensionRevision .spec.phases optional (operator-framework#2230)

Given that at the creation we can create a revision with no phases, we should
declare `.spec.phases` as optional.

Added initial set of unit tests to assert CER .spec immutability.

Author: pedjak

api/v1/clusterextensionrevision_types.go

@@ -55,13 +55,13 @@ type ClusterExtensionRevisionSpec struct {
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="revision is immutable"
 	Revision int64 `json:"revision"`
 	// Phases are groups of objects that will be applied at the same time.
-	// All objects in the a phase will have to pass their probes in order to progress to the next phase.
+	// All objects in the phase will have to pass their probes in order to progress to the next phase.
 	//
-	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf.size() == 0", message="phases is immutable"
 	// +listType=map
 	// +listMapKey=name
-	Phases []ClusterExtensionRevisionPhase `json:"phases"`
+	// +optional
+	Phases []ClusterExtensionRevisionPhase `json:"phases,omitempty"`
 	// Previous references previous revisions that objects can be adopted from.
 	//
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="previous is immutable"
@@ -104,6 +104,7 @@ type ClusterExtensionRevisionObject struct {
 	// already existing on the cluster or even owned by another controller.
 	//
 	// +kubebuilder:default="Prevent"
+	// +optional
 	CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"`
 }
 

api/v1/clusterextensionrevision_types_test.go

@@ -0,0 +1,94 @@
+package v1
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestClusterExtensionRevisionImmutability(t *testing.T) {
+	c := newClient(t)
+	ctx := context.Background()
+	i := 0
+	for name, tc := range map[string]struct {
+		spec       ClusterExtensionRevisionSpec
+		updateFunc func(*ClusterExtensionRevision)
+		allowed    bool
+	}{
+		"revision is immutable": {
+			spec: ClusterExtensionRevisionSpec{
+				Revision: 1,
+			},
+			updateFunc: func(cer *ClusterExtensionRevision) {
+				cer.Spec.Revision = 2
+			},
+		},
+		"phases may be initially empty": {
+			spec: ClusterExtensionRevisionSpec{
+				Phases: []ClusterExtensionRevisionPhase{},
+			},
+			updateFunc: func(cer *ClusterExtensionRevision) {
+				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
+					{
+						Name:    "foo",
+						Objects: []ClusterExtensionRevisionObject{},
+					},
+				}
+			},
+			allowed: true,
+		},
+		"phases may be initially unset": {
+			spec: ClusterExtensionRevisionSpec{},
+			updateFunc: func(cer *ClusterExtensionRevision) {
+				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
+					{
+						Name:    "foo",
+						Objects: []ClusterExtensionRevisionObject{},
+					},
+				}
+			},
+			allowed: true,
+		},
+		"phases are immutable if not empty": {
+			spec: ClusterExtensionRevisionSpec{
+				Phases: []ClusterExtensionRevisionPhase{
+					{
+						Name:    "foo",
+						Objects: []ClusterExtensionRevisionObject{},
+					},
+				},
+			},
+			updateFunc: func(cer *ClusterExtensionRevision) {
+				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
+					{
+						Name:    "foo2",
+						Objects: []ClusterExtensionRevisionObject{},
+					},
+				}
+			},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			cer := &ClusterExtensionRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf("foo%d", i),
+				},
+				Spec: tc.spec,
+			}
+			i = i + 1
+			require.NoError(t, c.Create(ctx, cer))
+			tc.updateFunc(cer)
+			err := c.Update(ctx, cer)
+			if tc.allowed && err != nil {
+				t.Fatal("expected update to succeed, but got:", err)
+			}
+			if !tc.allowed && !errors.IsInvalid(err) {
+				t.Fatal("expected update to fail due to invalid payload, but got:", err)
+			}
+		})
+	}
+}

api/v1/suite_test.go

@@ -0,0 +1,93 @@
+/*
+Copyright 2025.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1
+
+import (
+	"log"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	apimachineryruntime "k8s.io/apimachinery/pkg/runtime"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/envtest"
+)
+
+func newScheme(t *testing.T) *apimachineryruntime.Scheme {
+	sch := apimachineryruntime.NewScheme()
+	require.NoError(t, AddToScheme(sch))
+	return sch
+}
+
+func newClient(t *testing.T) client.Client {
+	cl, err := client.New(config, client.Options{Scheme: newScheme(t)})
+	require.NoError(t, err)
+	require.NotNil(t, cl)
+	return cl
+}
+
+var config *rest.Config
+
+func TestMain(m *testing.M) {
+	testEnv := &envtest.Environment{
+		CRDDirectoryPaths: []string{
+			filepath.Join("..", "..", "helm", "olmv1", "base", "operator-controller", "crd", "experimental"),
+		},
+		ErrorIfCRDPathMissing: true,
+	}
+
+	// ENVTEST-based tests require specific binaries. By default, these binaries are located
+	// in paths defined by controller-runtime. However, the `BinaryAssetsDirectory` needs
+	// to be explicitly set when running tests directly (e.g., debugging tests in an IDE)
+	// without using the Makefile targets.
+	//
+	// This is equivalent to configuring your IDE to export the `KUBEBUILDER_ASSETS` environment
+	// variable before each test execution. The following function simplifies this process
+	// by handling the configuration for you.
+	//
+	// To ensure the binaries are in the expected path without manual configuration, run:
+	// `make envtest-k8s-bins`
+	if getFirstFoundEnvTestBinaryDir() != "" {
+		testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir()
+	}
+
+	var err error
+	config, err = testEnv.Start()
+	utilruntime.Must(err)
+	if config == nil {
+		log.Panic("expected cfg to not be nil")
+	}
+
+	code := m.Run()
+	utilruntime.Must(testEnv.Stop())
+	os.Exit(code)
+}
+
+// getFirstFoundEnvTestBinaryDir finds and returns the first directory under the given path.
+func getFirstFoundEnvTestBinaryDir() string {
+	basePath := filepath.Join("..", "..", "bin", "envtest-binaries", "k8s")
+	entries, _ := os.ReadDir(basePath)
+	for _, entry := range entries {
+		if entry.IsDir() {
+			return filepath.Join(basePath, entry.Name())
+		}
+	}
+	return ""
+}

helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml

@@ -57,7 +57,7 @@ spec:
               phases:
                 description: |-
                   Phases are groups of objects that will be applied at the same time.
-                  All objects in the a phase will have to pass their probes in order to progress to the next phase.
+                  All objects in the phase will have to pass their probes in order to progress to the next phase.
                 items:
                   description: |-
                     ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time.
@@ -130,7 +130,6 @@ spec:
                 - message: revision is immutable
                   rule: self == oldSelf
             required:
-            - phases
             - revision
             type: object
           status:

manifests/experimental-e2e.yaml

@@ -648,7 +648,7 @@ spec:
               phases:
                 description: |-
                   Phases are groups of objects that will be applied at the same time.
-                  All objects in the a phase will have to pass their probes in order to progress to the next phase.
+                  All objects in the phase will have to pass their probes in order to progress to the next phase.
                 items:
                   description: |-
                     ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time.
@@ -721,7 +721,6 @@ spec:
                 - message: revision is immutable
                   rule: self == oldSelf
             required:
-            - phases
             - revision
             type: object
           status:

manifests/experimental.yaml

@@ -613,7 +613,7 @@ spec:
               phases:
                 description: |-
                   Phases are groups of objects that will be applied at the same time.
-                  All objects in the a phase will have to pass their probes in order to progress to the next phase.
+                  All objects in the phase will have to pass their probes in order to progress to the next phase.
                 items:
                   description: |-
                     ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time.
@@ -686,7 +686,6 @@ spec:
                 - message: revision is immutable
                   rule: self == oldSelf
             required:
-            - phases
             - revision
             type: object
           status: