ClusterExtensionRevision API Updates

Fixing some missing flags and godoc comments brought up via API review.

Signed-off-by: Daniel Franz <dfranz@redhat.com>

Author: dtfranz

api/v1/clusterextensionrevision_types.go

@@ -42,14 +42,18 @@ const (
 type ClusterExtensionRevisionSpec struct {
 	// lifecycleState specifies the lifecycle state of the ClusterExtensionRevision.
 	//
-	// When set to "Active" (the default), the revision is actively managed and reconciled.
+	// When set to "Active", the revision is actively managed and reconciled.
 	// When set to "Archived", the revision is inactive and any resources not managed by a subsequent revision are deleted.
 	// The revision is removed from the owner list of all objects previously under management.
 	// All objects that did not transition to a succeeding revision are deleted.
 	//
 	// Once a revision is set to "Archived", it cannot be un-archived.
 	//
-	// +kubebuilder:default="Active"
+	// It is possible for more than one revision to be "Active" simultaneously. This will occur when
+	// moving from one revision to another. The old revision will not be set to "Archived" until the
+	// new revision has been completely rolled out.
+	//
+	// +required
 	// +kubebuilder:validation:Enum=Active;Archived
 	// +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Archived' && oldSelf == self", message="cannot un-archive"
 	LifecycleState ClusterExtensionRevisionLifecycleState `json:"lifecycleState,omitempty"`
@@ -66,7 +70,8 @@ type ClusterExtensionRevisionSpec struct {
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="revision is immutable"
 	Revision int64 `json:"revision"`
 
-	// phases is an optional, immutable list of phases that group objects to be applied together.
+	// inlinePhases is an optional, immutable list of phases that group objects to be applied together. Phases declared in
+	// this manner are included in the ClusterExtensionRevision as-is, as opposed to being sharded across multiple resources.
 	//
 	// Objects are organized into phases based on their Group-Kind. Common phases include:
 	//   - namespaces: Namespace objects
@@ -80,13 +85,16 @@ type ClusterExtensionRevisionSpec struct {
 	// All objects in a phase are applied in no particular order.
 	// The revision progresses to the next phase only after all objects in the current phase pass their readiness probes.
 	//
-	// Once set, even if empty, the phases field is immutable.
+	// Once set, even if empty, the inlinePhases field is immutable.
+	//
+	// Each phase in the list must have a unique name. The minimum number of inlinePhases is 1, and the maximum is 20.
 	//
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf.size() == 0", message="phases is immutable"
+	// +kubebuilder:validation:MaxItems=20
 	// +listType=map
 	// +listMapKey=name
 	// +optional
-	Phases []ClusterExtensionRevisionPhase `json:"phases,omitempty"`
+	InlinePhases []ClusterExtensionRevisionPhase `json:"inlinePhases,omitempty"`
 
 	// progressDeadlineMinutes is an optional field that defines the maximum period
 	// of time in minutes after which an installation should be considered failed and
@@ -125,13 +133,17 @@ type ClusterExtensionRevisionPhase struct {
 	//
 	// [RFC 1123]: https://tools.ietf.org/html/rfc1123
 	//
+	// +required
+	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=63
-	// +kubebuilder:validation:Pattern=`^[a-z]([-a-z0-9]*[a-z0-9])?$`
+	// +kubebuilder:validation:XValidation:rule=`!format.dns1123Label().validate(self).hasValue()`,message="the value must consist of only lowercase alphanumeric characters and hyphens, and must start with an alphabetic character and end with an alphanumeric character."
 	Name string `json:"name"`
 
 	// objects is a required list of all Kubernetes objects that belong to this phase.
 	//
-	// All objects in this list are applied to the cluster in no particular order.
+	// All objects in this list are applied to the cluster in no particular order. The maximum number of objects per phase is 50.
+	// +required
+	// +kubebuilder:validation:MaxItems=50
 	Objects []ClusterExtensionRevisionObject `json:"objects"`
 }
 
@@ -149,6 +161,8 @@ type ClusterExtensionRevisionObject struct {
 	// collisionProtection controls whether the operator can adopt and modify objects
 	// that already exist on the cluster.
 	//
+	// Allowed values are: "Prevent", "IfNoController", and "None".
+	//
 	// When set to "Prevent" (the default), the operator only manages objects it created itself.
 	// This prevents ownership collisions.
 	//
@@ -161,9 +175,8 @@ type ClusterExtensionRevisionObject struct {
 	// Use this setting with extreme caution as it may cause multiple controllers to fight over
 	// the same resource, resulting in increased load on the API server and etcd.
 	//
-	// +kubebuilder:default="Prevent"
+	// +required
 	// +kubebuilder:validation:Enum=Prevent;IfNoController;None
-	// +optional
 	CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"`
 }
 

api/v1/clusterextensionrevision_types_test.go

@@ -21,19 +21,21 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
 	}{
 		"revision is immutable": {
 			spec: ClusterExtensionRevisionSpec{
-				Revision: 1,
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
 			},
 			updateFunc: func(cer *ClusterExtensionRevision) {
 				cer.Spec.Revision = 2
 			},
 		},
 		"phases may be initially empty": {
 			spec: ClusterExtensionRevisionSpec{
-				Revision: 1,
-				Phases:   []ClusterExtensionRevisionPhase{},
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
+				InlinePhases:   []ClusterExtensionRevisionPhase{},
 			},
 			updateFunc: func(cer *ClusterExtensionRevision) {
-				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
+				cer.Spec.InlinePhases = []ClusterExtensionRevisionPhase{
 					{
 						Name:    "foo",
 						Objects: []ClusterExtensionRevisionObject{},
@@ -44,10 +46,11 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
 		},
 		"phases may be initially unset": {
 			spec: ClusterExtensionRevisionSpec{
-				Revision: 1,
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
 			},
 			updateFunc: func(cer *ClusterExtensionRevision) {
-				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
+				cer.Spec.InlinePhases = []ClusterExtensionRevisionPhase{
 					{
 						Name:    "foo",
 						Objects: []ClusterExtensionRevisionObject{},
@@ -58,16 +61,17 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
 		},
 		"phases are immutable if not empty": {
 			spec: ClusterExtensionRevisionSpec{
-				Revision: 1,
-				Phases: []ClusterExtensionRevisionPhase{
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
+				InlinePhases: []ClusterExtensionRevisionPhase{
 					{
 						Name:    "foo",
 						Objects: []ClusterExtensionRevisionObject{},
 					},
 				},
 			},
 			updateFunc: func(cer *ClusterExtensionRevision) {
-				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
+				cer.Spec.InlinePhases = []ClusterExtensionRevisionPhase{
 					{
 						Name:    "foo2",
 						Objects: []ClusterExtensionRevisionObject{},
@@ -107,20 +111,87 @@ func TestClusterExtensionRevisionValidity(t *testing.T) {
 	}{
 		"revision cannot be negative": {
 			spec: ClusterExtensionRevisionSpec{
-				Revision: -1,
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       -1,
 			},
 			valid: false,
 		},
 		"revision cannot be zero": {
-			spec:  ClusterExtensionRevisionSpec{},
+			spec: ClusterExtensionRevisionSpec{
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+			},
 			valid: false,
 		},
 		"revision must be positive": {
 			spec: ClusterExtensionRevisionSpec{
-				Revision: 1,
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
 			},
 			valid: true,
 		},
+		"lifecycleState must be set": {
+			spec: ClusterExtensionRevisionSpec{
+				Revision: 1,
+			},
+			valid: false,
+		},
+		"inlinePhases must have no more than 20 phases": {
+			spec: ClusterExtensionRevisionSpec{
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
+				InlinePhases:   make([]ClusterExtensionRevisionPhase, 21),
+			},
+			valid: false,
+		},
+		"inlinePhases entries must have no more than 50 objects": {
+			spec: ClusterExtensionRevisionSpec{
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
+				InlinePhases: []ClusterExtensionRevisionPhase{
+					{
+						Name:    "too-many-objects",
+						Objects: make([]ClusterExtensionRevisionObject, 51),
+					},
+				},
+			},
+			valid: false,
+		},
+		"inlinePhases entry names cannot be empty": {
+			spec: ClusterExtensionRevisionSpec{
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
+				InlinePhases: []ClusterExtensionRevisionPhase{
+					{
+						Name: "",
+					},
+				},
+			},
+			valid: false,
+		},
+		"inlinePhases entry names cannot start with symbols": {
+			spec: ClusterExtensionRevisionSpec{
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
+				InlinePhases: []ClusterExtensionRevisionPhase{
+					{
+						Name: "-invalid",
+					},
+				},
+			},
+			valid: false,
+		},
+		"inlinePhases entry names cannot start with numeric characters": {
+			spec: ClusterExtensionRevisionSpec{
+				LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				Revision:       1,
+				InlinePhases: []ClusterExtensionRevisionPhase{
+					{
+						Name: "1-invalid",
+					},
+				},
+			},
+			valid: false,
+		},
 	} {
 		t.Run(name, func(t *testing.T) {
 			cer := &ClusterExtensionRevision{

api/v1/validation_test.go

@@ -91,6 +91,7 @@ func TestValidate(t *testing.T) {
 		"ClusterExtensionRevision: invalid progress deadline < 10": {
 			args: args{
 				object: ClusterExtensionRevisionSpec{
+					LifecycleState:          ClusterExtensionRevisionLifecycleStateActive,
 					ProgressDeadlineMinutes: 9,
 				},
 			},
@@ -99,6 +100,7 @@ func TestValidate(t *testing.T) {
 		"ClusterExtensionRevision: valid progress deadline = 10": {
 			args: args{
 				object: ClusterExtensionRevisionSpec{
+					LifecycleState:          ClusterExtensionRevisionLifecycleStateActive,
 					ProgressDeadlineMinutes: 10,
 				},
 			},
@@ -107,6 +109,7 @@ func TestValidate(t *testing.T) {
 		"ClusterExtensionRevision: valid progress deadline = 360": {
 			args: args{
 				object: ClusterExtensionRevisionSpec{
+					LifecycleState:          ClusterExtensionRevisionLifecycleStateActive,
 					ProgressDeadlineMinutes: 360,
 				},
 			},
@@ -115,6 +118,7 @@ func TestValidate(t *testing.T) {
 		"ClusterExtensionRevision: valid progress deadline = 720": {
 			args: args{
 				object: ClusterExtensionRevisionSpec{
+					LifecycleState:          ClusterExtensionRevisionLifecycleStateActive,
 					ProgressDeadlineMinutes: 720,
 				},
 			},
@@ -123,14 +127,17 @@ func TestValidate(t *testing.T) {
 		"ClusterExtensionRevision: invalid progress deadline > 720": {
 			args: args{
 				object: ClusterExtensionRevisionSpec{
+					LifecycleState:          ClusterExtensionRevisionLifecycleStateActive,
 					ProgressDeadlineMinutes: 721,
 				},
 			},
 			want: want{valid: false},
 		},
 		"ClusterExtensionRevision: no progress deadline set": {
 			args: args{
-				object: ClusterExtensionRevisionSpec{},
+				object: ClusterExtensionRevisionSpec{
+					LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
+				},
 			},
 			want: want{valid: true},
 		},