Address comments

internal/test/builder/v1beta2_transition.go

@@ -207,11 +207,20 @@ type Phase2ObjStatusDeprecatedV1Beta1 struct {
 
 // GetConditions returns the set of conditions for this object.
 func (o *Phase2Obj) GetConditions() clusterv1.Conditions {
+	if o.Status.Deprecated == nil || o.Status.Deprecated.V1Beta1 == nil {
+		return nil
+	}
 	return o.Status.Deprecated.V1Beta1.Conditions
 }
 
 // SetConditions sets the conditions on this object.
 func (o *Phase2Obj) SetConditions(conditions clusterv1.Conditions) {
+	if o.Status.Deprecated == nil && conditions != nil {
+		o.Status.Deprecated = &Phase2ObjStatusDeprecated{V1Beta1: &Phase2ObjStatusDeprecatedV1Beta1{}}
+	}
+	if o.Status.Deprecated.V1Beta1 == nil && conditions != nil {
+		o.Status.Deprecated.V1Beta1 = &Phase2ObjStatusDeprecatedV1Beta1{}
+	}
 	o.Status.Deprecated.V1Beta1.Conditions = conditions
 }
 

util/conditions/v1beta2/options.go

@@ -86,3 +86,18 @@ type StepCounter bool
 func (t StepCounter) ApplyToSummary(opts *SummaryOptions) {
 	opts.stepCounter = bool(t)
 }
+
+// OwnedConditionTypes allows to define condition types owned by the controller when performing patch apply.
+// In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
+func OwnedConditionTypes(conditionTypes ...string) ApplyOption {
+	return func(c *applyOptions) {
+		c.ownedConditionTypes = conditionTypes
+	}
+}
+
+// ForceOverwrite instructs patch apply to always use the value provided by the controller (no matter of what value exists currently).
+func ForceOverwrite(v bool) ApplyOption {
+	return func(c *applyOptions) {
+		c.forceOverwrite = v
+	}
+}

util/conditions/v1beta2/patch.go

@@ -99,13 +99,13 @@ func NewPatch(before, after runtime.Object) (Patch, error) {
 
 // applyOptions allows to set strategies for patch apply.
 type applyOptions struct {
-	ownedConditions []string
-	forceOverwrite  bool
+	ownedConditionTypes []string
+	forceOverwrite      bool
 }
 
-func (o *applyOptions) isOwnedCondition(t string) bool {
-	for _, i := range o.ownedConditions {
-		if i == t {
+func (o *applyOptions) isOwnedConditionType(conditionType string) bool {
+	for _, i := range o.ownedConditionTypes {
+		if i == conditionType {
 			return true
 		}
 	}
@@ -115,21 +115,6 @@ func (o *applyOptions) isOwnedCondition(t string) bool {
 // ApplyOption defines an option for applying a condition patch.
 type ApplyOption func(*applyOptions)
 
-// WithOwnedConditions allows to define condition types owned by the controller.
-// In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
-func WithOwnedConditions(t ...string) ApplyOption {
-	return func(c *applyOptions) {
-		c.ownedConditions = t
-	}
-}
-
-// WithForceOverwrite In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
-func WithForceOverwrite(v bool) ApplyOption {
-	return func(c *applyOptions) {
-		c.forceOverwrite = v
-	}
-}
-
 // Apply executes a three-way merge of a list of Patch.
 // When merge conflicts are detected (latest deviated from before in an incompatible way), an error is returned.
 func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error {
@@ -157,14 +142,14 @@ func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error {
 		switch conditionPatch.Op {
 		case AddConditionPatch:
 			// If the conditions is owned, always keep the after value.
-			if applyOpt.forceOverwrite || applyOpt.isOwnedCondition(conditionPatch.After.Type) {
+			if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.After.Type) {
 				meta.SetStatusCondition(&latestConditions, *conditionPatch.After)
 				continue
 			}
 
 			// If the condition is already on latest, check if latest and after agree on the change; if not, this is a conflict.
 			if latestCondition := meta.FindStatusCondition(latestConditions, conditionPatch.After.Type); latestCondition != nil {
-				// If latest and after agree on the change, then it is a conflict.
+				// If latest and after disagreeWithForceOverwrite on the change, then it is a conflict.
 				if !hasSameState(latestCondition, conditionPatch.After) {
 					return errors.Errorf("error patching conditions: The condition %q was modified by a different process and this caused a merge/AddCondition conflict: %v", conditionPatch.After.Type, cmp.Diff(latestCondition, conditionPatch.After))
 				}
@@ -177,7 +162,7 @@ func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error {
 
 		case ChangeConditionPatch:
 			// If the conditions is owned, always keep the after value.
-			if applyOpt.forceOverwrite || applyOpt.isOwnedCondition(conditionPatch.After.Type) {
+			if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.After.Type) {
 				meta.SetStatusCondition(&latestConditions, *conditionPatch.After)
 				continue
 			}
@@ -209,7 +194,7 @@ func (p Patch) Apply(latest runtime.Object, options ...ApplyOption) error {
 			}
 
 			// If the conditions is owned, always keep the after value (condition should be deleted).
-			if applyOpt.forceOverwrite || applyOpt.isOwnedCondition(conditionPatch.Before.Type) {
+			if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.Before.Type) {
 				meta.RemoveStatusCondition(&latestConditions, conditionPatch.Before.Type)
 				continue
 			}

util/conditions/v1beta2/patch_test.go

@@ -129,9 +129,10 @@ func TestNewPatch(t *testing.T) {
 }
 
 func TestApply(t *testing.T) {
-	fooTrue := metav1.Condition{Type: "foo", Status: metav1.ConditionTrue}
-	fooFalse := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse}
-	fooFalse2 := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, Reason: "Something else"}
+	now := metav1.Now().Rfc3339Copy()
+	fooTrue := metav1.Condition{Type: "foo", Status: metav1.ConditionTrue, LastTransitionTime: now}
+	fooFalse := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, LastTransitionTime: now}
+	fooFalse2 := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, Reason: "Something else", LastTransitionTime: now}
 
 	tests := []struct {
 		name    string
@@ -195,7 +196,7 @@ func TestApply(t *testing.T) {
 			before:  objectWithConditions(),
 			after:   objectWithConditions(fooTrue),
 			latest:  objectWithConditions(fooFalse),
-			options: []ApplyOption{WithForceOverwrite(true)},
+			options: []ApplyOption{ForceOverwrite(true)},
 			want:    []metav1.Condition{fooTrue}, // after condition should be kept in case of error
 			wantErr: false,
 		},
@@ -204,7 +205,7 @@ func TestApply(t *testing.T) {
 			before:  objectWithConditions(),
 			after:   objectWithConditions(fooTrue),
 			latest:  objectWithConditions(fooFalse),
-			options: []ApplyOption{WithOwnedConditions("foo")},
+			options: []ApplyOption{OwnedConditionTypes("foo")},
 			want:    []metav1.Condition{fooTrue}, // after condition should be kept in case of error
 			wantErr: false,
 		},
@@ -237,17 +238,17 @@ func TestApply(t *testing.T) {
 			before:  objectWithConditions(fooTrue),
 			after:   objectWithConditions(),
 			latest:  objectWithConditions(fooFalse),
-			options: []ApplyOption{WithForceOverwrite(true)},
-			want:    []metav1.Condition{}, // after condition should be kept in case of error
+			options: []ApplyOption{ForceOverwrite(true)},
+			want:    []metav1.Condition{},
 			wantErr: false,
 		},
 		{
 			name:    "Remove: When a condition already exists but with conflicts, it should not error if the condition is owned",
 			before:  objectWithConditions(fooTrue),
 			after:   objectWithConditions(),
 			latest:  objectWithConditions(fooFalse),
-			options: []ApplyOption{WithOwnedConditions("foo")},
-			want:    []metav1.Condition{}, // after condition should be kept in case of error
+			options: []ApplyOption{OwnedConditionTypes("foo")},
+			want:    []metav1.Condition{},
 			wantErr: false,
 		},
 		{
@@ -279,17 +280,17 @@ func TestApply(t *testing.T) {
 			before:  objectWithConditions(fooFalse),
 			after:   objectWithConditions(fooFalse2),
 			latest:  objectWithConditions(fooTrue),
-			options: []ApplyOption{WithForceOverwrite(true)},
-			want:    []metav1.Condition{fooFalse2}, // after condition should be kept in case of error
+			options: []ApplyOption{ForceOverwrite(true)},
+			want:    []metav1.Condition{fooFalse2},
 			wantErr: false,
 		},
 		{
 			name:    "Change: When a condition exists with conflicts but there is no agreement on the final state, it should not error if the condition is owned",
 			before:  objectWithConditions(fooFalse),
 			after:   objectWithConditions(fooFalse2),
 			latest:  objectWithConditions(fooTrue),
-			options: []ApplyOption{WithOwnedConditions("foo")},
-			want:    []metav1.Condition{fooFalse2}, // after condition should be kept in case of error
+			options: []ApplyOption{OwnedConditionTypes("foo")},
+			want:    []metav1.Condition{fooFalse2},
 			wantErr: false,
 		},
 		{
@@ -305,17 +306,17 @@ func TestApply(t *testing.T) {
 			before:  objectWithConditions(fooTrue),
 			after:   objectWithConditions(fooFalse),
 			latest:  objectWithConditions(),
-			options: []ApplyOption{WithForceOverwrite(true)},
-			want:    []metav1.Condition{fooFalse}, // after condition should be kept in case of error
+			options: []ApplyOption{ForceOverwrite(true)},
+			want:    []metav1.Condition{fooFalse},
 			wantErr: false,
 		},
 		{
 			name:    "Change: When a condition was deleted, it should not error if the condition is owned",
 			before:  objectWithConditions(fooTrue),
 			after:   objectWithConditions(fooFalse),
 			latest:  objectWithConditions(),
-			options: []ApplyOption{WithOwnedConditions("foo")},
-			want:    []metav1.Condition{fooFalse}, // after condition should be kept in case of error
+			options: []ApplyOption{OwnedConditionTypes("foo")},
+			want:    []metav1.Condition{fooFalse},
 			wantErr: false,
 		},
 		{
@@ -345,7 +346,7 @@ func TestApply(t *testing.T) {
 			gotConditions, err := GetAll(tt.latest)
 			g.Expect(err).ToNot(HaveOccurred())
 
-			g.Expect(gotConditions).To(MatchConditions(tt.want, IgnoreLastTransitionTime(true)))
+			g.Expect(gotConditions).To(MatchConditions(tt.want))
 		})
 	}
 }

util/conditions/v1beta2/setter.go

@@ -152,8 +152,16 @@ func setToTypedObject(obj runtime.Object, conditions []metav1.Condition) error {
 		}
 
 		v1beta2Elem := v1beta2Field.Elem()
+
+		// If v1beta2Elem is nil (not a valid value), initialize it only if the condition list is not nil.
+		// NOTE: this makes the UX simpler (the caller should not take care of initializing the v1beta2 field before calling set).
 		if !v1beta2Elem.IsValid() {
-			return errors.Errorf("%s.status.v1beta2 must be a valid value (non zero value of its type)", ownerInfo)
+			if conditions == nil {
+				return nil
+			}
+			v1beta2Value := reflect.New(v1beta2Field.Type().Elem())
+			v1beta2Field.Set(v1beta2Value)
+			v1beta2Elem = v1beta2Field.Elem()
 		}
 
 		if conditionField := v1beta2Elem.FieldByName("Conditions"); conditionField != (reflect.Value{}) {

util/conditions/v1beta2/setter_test.go

@@ -86,7 +86,48 @@ func TestSetAll(t *testing.T) {
 		g.Expect(err).To(HaveOccurred()) // Can't set legacy conditions.
 	})
 
-	t.Run("v1beta1 object with both legacy and v1beta2 conditions", func(t *testing.T) {
+	t.Run("v1beta1 object with both legacy and v1beta2 conditions (v1Beta2 nil)", func(t *testing.T) {
+		g := NewWithT(t)
+		foo := &builder.Phase1Obj{
+			Status: builder.Phase1ObjStatus{
+				Conditions: clusterv1.Conditions{
+					{
+						Type:               "barCondition",
+						Status:             corev1.ConditionFalse,
+						LastTransitionTime: now,
+					},
+				},
+				// V1Beta2 is nil, but it should be automatically initialized
+			},
+		}
+
+		conditions := cloneConditions()
+		err := SetAll(foo, conditions)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(foo.Status.V1Beta2.Conditions).To(Equal(conditions), cmp.Diff(foo.Status.V1Beta2.Conditions, conditions))
+	})
+
+	t.Run("v1beta1 object with both legacy and v1beta2 conditions (v1Beta2 nil should remain nil if nil conditions is passed)", func(t *testing.T) {
+		g := NewWithT(t)
+		foo := &builder.Phase1Obj{
+			Status: builder.Phase1ObjStatus{
+				Conditions: clusterv1.Conditions{
+					{
+						Type:               "barCondition",
+						Status:             corev1.ConditionFalse,
+						LastTransitionTime: now,
+					},
+				},
+				// V1Beta2 is nil, but it should be automatically initialized
+			},
+		}
+
+		err := SetAll(foo, nil)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(foo.Status.V1Beta2).To(BeNil())
+	})
+
+	t.Run("v1beta1 object with both legacy and v1beta2 conditions (v1Beta2 not nil)", func(t *testing.T) {
 		g := NewWithT(t)
 		foo := &builder.Phase1Obj{
 			Status: builder.Phase1ObjStatus{

util/patch/options.go

@@ -38,9 +38,9 @@ type HelperOptions struct {
 	// In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
 	OwnedConditions []clusterv1.ConditionType
 
-	// OwnedV1Beta1Conditions defines condition types owned by the controller.
+	// OwnedV1Beta2Conditions defines condition types owned by the controller.
 	// In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
-	OwnedV1Beta1Conditions []string
+	OwnedV1Beta2Conditions []string
 }
 
 // WithForceOverwriteConditions allows the patch helper to overwrite conditions in case of conflicts.
@@ -72,13 +72,13 @@ func (w WithOwnedConditions) ApplyToHelper(in *HelperOptions) {
 	in.OwnedConditions = w.Conditions
 }
 
-// WithOwnedV1Beta1Conditions allows to define condition types owned by the controller.
+// WithOwnedV1Beta2Conditions allows to define condition types owned by the controller.
 // In case of conflicts for the owned conditions, the patch helper will always use the value provided by the controller.
-type WithOwnedV1Beta1Conditions struct {
+type WithOwnedV1Beta2Conditions struct {
 	Conditions []string
 }
 
 // ApplyToHelper applies this configuration to the given HelperOptions.
-func (w WithOwnedV1Beta1Conditions) ApplyToHelper(in *HelperOptions) {
-	in.OwnedV1Beta1Conditions = w.Conditions
+func (w WithOwnedV1Beta2Conditions) ApplyToHelper(in *HelperOptions) {
+	in.OwnedV1Beta2Conditions = w.Conditions
 }