Address comments

internal/test/builder/v1beta2_transition.go

@@ -223,13 +223,16 @@ 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{}
+		o.Status.Deprecated = &Phase2ObjStatusDeprecated{V1Beta1: &Phase2ObjStatusDeprecatedV1Beta1{}}
 	}
 	if o.Status.Deprecated.V1Beta1 == nil && conditions != nil {
 		o.Status.Deprecated.V1Beta1 = &Phase2ObjStatusDeprecatedV1Beta1{}

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/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
 }

util/patch/patch.go

@@ -152,7 +152,7 @@ func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) e
 	// Given that we pass in metadata.resourceVersion to perform a 3-way-merge conflict resolution,
 	// patching conditions first avoids an extra loop if spec or status patch succeeds first
 	// given that causes the resourceVersion to mutate.
-	if err := h.patchStatusConditions(ctx, obj, options.ForceOverwriteConditions, options.OwnedConditions, options.OwnedV1Beta1Conditions); err != nil {
+	if err := h.patchStatusConditions(ctx, obj, options.ForceOverwriteConditions, options.OwnedConditions, options.OwnedV1Beta2Conditions); err != nil {
 		errs = append(errs, err)
 	}
 	// Then proceed to patch the rest of the object.
@@ -206,7 +206,7 @@ func (h *Helper) patchStatus(ctx context.Context, obj client.Object) error {
 // Condition changes are then applied to the latest version of the object, and if there are
 // no unresolvable conflicts, the patch is sent again.
 func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, forceOverwrite bool, ownedConditions []clusterv1.ConditionType, ownedV1beta2Conditions []string) error {
-	// Nothing to do if the object doesn't have conditions.
+	// Nothing to do if the object doesn't have conditions (doesn't have conditions identified as needing a special treatment).
 	if len(h.clusterv1ConditionsFields) == 0 && len(h.metav1ConditionsFields) == 0 {
 		return nil
 	}
@@ -220,26 +220,25 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
 		// interface any longer, although this shouldn't happen because we already check when creating the patcher.
 		before, ok := h.beforeObject.(conditions.Getter)
 		if !ok {
-			return errors.Errorf("object %s doesn't satisfy conditions.Getter, cannot patch", before.GetObjectKind())
+			return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot patch", h.gvk.Kind, klog.KObj(before))
 		}
 		after, ok := obj.(conditions.Getter)
 		if !ok {
-			return errors.Errorf("object %s doesn't satisfy conditions.Getter, cannot patch", after.GetObjectKind())
+			return errors.Errorf("%s %s doesn't satisfy conditions.Getter, cannot compute patch", h.gvk.Kind, after.GetObjectKind())
 		}
 
-		// Store the diff from the before/after object, and return early if there are no changes.
 		diff, err := conditions.NewPatch(
 			before,
 			after,
 		)
 		if err != nil {
-			return errors.Wrapf(err, "object can not be patched")
+			return errors.Wrapf(err, "%s can not be patched", h.gvk.Kind)
 		}
 		if !diff.IsZero() {
 			clusterv1ApplyPatch = func(latest client.Object) error {
 				latestSetter, ok := latest.(conditions.Setter)
 				if !ok {
-					return errors.Errorf("object %s doesn't satisfy conditions.Setter, cannot patch", before.GetObjectKind())
+					return errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot apply patch", h.gvk.Kind, before.GetObjectKind())
 				}
 
 				return diff.Apply(latestSetter, conditions.WithForceOverwrite(forceOverwrite), conditions.WithOwnedConditions(ownedConditions...))
@@ -250,22 +249,22 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
 	// If the object has metav1 conditions, create a function applying corresponding changes if any.
 	var metav1ApplyPatch func(client.Object) error
 	if len(h.metav1ConditionsFields) > 0 {
-		// Store the diff from the before/after object, and return early if there are no changes.
 		diff, err := v1beta2conditions.NewPatch(
 			h.beforeObject,
 			obj,
 		)
 		if err != nil {
-			return errors.Wrapf(err, "object can not be patched")
+			return errors.Wrapf(err, "%s can not be patched", h.gvk.Kind)
 		}
 
 		if !diff.IsZero() {
 			metav1ApplyPatch = func(latest client.Object) error {
-				return diff.Apply(latest, v1beta2conditions.WithForceOverwrite(forceOverwrite), v1beta2conditions.WithOwnedConditions(ownedV1beta2Conditions...))
+				return diff.Apply(latest, v1beta2conditions.ForceOverwrite(forceOverwrite), v1beta2conditions.OwnedConditionTypes(ownedV1beta2Conditions...))
 			}
 		}
 	}
 
+	// No changes to apply, return early.
 	if clusterv1ApplyPatch == nil && metav1ApplyPatch == nil {
 		return nil
 	}
@@ -287,7 +286,7 @@ func (h *Helper) patchStatusConditions(ctx context.Context, obj client.Object, f
 	return wait.ExponentialBackoff(backoff, func() (bool, error) {
 		latest, ok := h.beforeObject.DeepCopyObject().(client.Object)
 		if !ok {
-			return false, errors.Errorf("object %s doesn't satisfy conditions.Setter, cannot patch", latest.GetObjectKind())
+			return false, errors.Errorf("%s %s doesn't satisfy conditions.Setter, cannot patch", h.gvk.Kind, latest.GetObjectKind())
 		}
 
 		// Get a new copy of the object.