Rename helpers

docs/proposals/20200506-conditions.md

@@ -243,8 +243,9 @@ When the above suffix are not adequate for a specific condition type, other suff
 (e.g. `Completed`, `Healthy`); however, it is recommended to balance this flexibility with the objective to provide
 a consistent condition naming across all the Cluster API objects.
 
-The `Severity` field MUST be set only when `Status=False` + positive polarity or when `Status=True` + negative polarity;
-severity it is designed to provide a standard classification of possible conditions failure `Reason`. 
+The `Severity` field MUST be set only when `Status=False` for conditions with positive polarity, or when `Status=True`
+for conditions with negative polarity; severity is designed to provide a standard classification of possible
+conditions failure `Reason`. 
 
 Please note that the combination of `Reason` and `Severity` gives different meaning to a condition failure
 allowing to detect when a long-running task is still ongoing:
@@ -473,7 +474,7 @@ enhance the condition utilities to handle those situations in a generalized way.
 - Risk: This proposal aims to be consistent with the target state of conditions in Kubernetes, but this
   is still under definition (see [KEP](https://github.com/kubernetes/enhancements/pull/1624)).
   - Mitigation: Periodically re-evaluate this proposal vs the Kubernetes KEP. 
-    -  2024-05-03: Change allowing conditions with negative polarity goes into this direction
+    -  2024-05-03: Change to allow conditions without positive polarity goes into this direction
 
 - Risk: Cluster API presents some specific challenges that are not common to the core Kubernetes objects.
   - Mitigation: To allow a minimal set of carefully evaluated differences between Cluster API and Kubernetes

util/conditions/getter_test.go

@@ -33,12 +33,12 @@ var (
 	falseWarning1 = FalseCondition("falseWarning1", "reason falseWarning1", clusterv1.ConditionSeverityWarning, "message falseWarning1")
 	falseError1   = FalseCondition("falseError1", "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1")
 
-	negativePolarityConditions = sets.New("positive-false1", "negative-unknown1", "negative-trueInfo1", "negative-trueWarning1", "negative-trueError1")
-	positiveFalse1             = PositiveFalseCondition("positive-false1")
-	negativeUnknown1           = UnknownCondition("negative-unknown1", "reason negative-unknown1", "message negative-unknown1")
-	negativeTrueInfo1          = NegativeTrueCondition("negative-trueInfo1", "reason negative-trueInfo1", clusterv1.ConditionSeverityInfo, "message negative-trueInfo1")
-	negativeTrueWarning1       = NegativeTrueCondition("negative-trueWarning1", "reason negative-trueWarning1", clusterv1.ConditionSeverityWarning, "message negative-trueWarning1")
-	negativeTrueError1         = NegativeTrueCondition("negative-trueError1", "reason negative-trueError1", clusterv1.ConditionSeverityError, "message negative-trueError1")
+	negativePolarityConditions       = sets.New("false1-negative-polarity", "unknown1-negative-polarity", "trueInfo1-negative-polarity", "trueWarning1-negative-polarity", "trueError1-negative-polarity")
+	false1WithNegativePolarity       = FalseConditionWithNegativePolarity("false1-negative-polarity")
+	unknown1WithNegativePolarity     = UnknownCondition("unknown1-negative-polarity", "reason unknown1-negative-polarity", "message unknown1-negative-polarity")
+	trueInfo1WithNegativePolarity    = TrueConditionWithNegativePolarity("trueInfo1-negative-polarity", "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity")
+	trueWarning1WithNegativePolarity = TrueConditionWithNegativePolarity("trueWarning1-negative-polarity", "reason trueWarning1-negative-polarity", clusterv1.ConditionSeverityWarning, "message trueWarning1-negative-polarity")
+	trueError1WithNegativePolarity   = TrueConditionWithNegativePolarity("trueError1-negative-polarity", "reason trueError1-negative-polarity", clusterv1.ConditionSeverityError, "message trueError1-negative-polarity")
 )
 
 func TestGetAndHas(t *testing.T) {
@@ -58,51 +58,51 @@ func TestGetAndHas(t *testing.T) {
 func TestIsMethods(t *testing.T) {
 	g := NewWithT(t)
 
-	obj := getterWithConditions(nil1, true1, unknown1, falseInfo1, falseWarning1, falseError1, positiveFalse1, negativeUnknown1, negativeTrueInfo1, negativeTrueWarning1, negativeTrueError1)
+	obj := getterWithConditions(nil1, true1, unknown1, falseInfo1, falseWarning1, falseError1, false1WithNegativePolarity, unknown1WithNegativePolarity, trueInfo1WithNegativePolarity, trueWarning1WithNegativePolarity, trueError1WithNegativePolarity)
 
 	// test isTrue
 	g.Expect(IsTrue(obj, "nil1")).To(BeFalse())
 	g.Expect(IsTrue(obj, "true1")).To(BeTrue())
 	g.Expect(IsTrue(obj, "falseInfo1")).To(BeFalse())
 	g.Expect(IsTrue(obj, "unknown1")).To(BeFalse())
-	g.Expect(IsTrue(obj, "positive-false1")).To(BeFalse())
-	g.Expect(IsTrue(obj, "negative-trueInfo1")).To(BeTrue())
-	g.Expect(IsTrue(obj, "negative-unknown1")).To(BeFalse())
+	g.Expect(IsTrue(obj, "false1-negative-polarity")).To(BeFalse())
+	g.Expect(IsTrue(obj, "trueInfo1-negative-polarity")).To(BeTrue())
+	g.Expect(IsTrue(obj, "unknown1-negative-polarity")).To(BeFalse())
 
 	// test isFalse
 	g.Expect(IsFalse(obj, "nil1")).To(BeFalse())
 	g.Expect(IsFalse(obj, "true1")).To(BeFalse())
 	g.Expect(IsFalse(obj, "falseInfo1")).To(BeTrue())
 	g.Expect(IsFalse(obj, "unknown1")).To(BeFalse())
-	g.Expect(IsFalse(obj, "positive-false1")).To(BeTrue())
-	g.Expect(IsFalse(obj, "negative-trueInfo1")).To(BeFalse())
-	g.Expect(IsFalse(obj, "negative-unknown1")).To(BeFalse())
+	g.Expect(IsFalse(obj, "false1-negative-polarity")).To(BeTrue())
+	g.Expect(IsFalse(obj, "trueInfo1-negative-polarity")).To(BeFalse())
+	g.Expect(IsFalse(obj, "unknown1-negative-polarity")).To(BeFalse())
 
 	// test isUnknown
 	g.Expect(IsUnknown(obj, "nil1")).To(BeTrue())
 	g.Expect(IsUnknown(obj, "true1")).To(BeFalse())
 	g.Expect(IsUnknown(obj, "falseInfo1")).To(BeFalse())
 	g.Expect(IsUnknown(obj, "unknown1")).To(BeTrue())
-	g.Expect(IsUnknown(obj, "positive-false1")).To(BeFalse())
-	g.Expect(IsUnknown(obj, "negative-trueInfo1")).To(BeFalse())
-	g.Expect(IsUnknown(obj, "negative-unknown1")).To(BeTrue())
+	g.Expect(IsUnknown(obj, "false1-negative-polarity")).To(BeFalse())
+	g.Expect(IsUnknown(obj, "trueInfo1-negative-polarity")).To(BeFalse())
+	g.Expect(IsUnknown(obj, "unknown1-negative-polarity")).To(BeTrue())
 
 	// test GetReason
 	g.Expect(GetReason(obj, "nil1")).To(Equal(""))
 	g.Expect(GetReason(obj, "falseInfo1")).To(Equal("reason falseInfo1"))
-	g.Expect(GetReason(obj, "negative-trueInfo1")).To(Equal("reason negative-trueInfo1"))
+	g.Expect(GetReason(obj, "trueInfo1-negative-polarity")).To(Equal("reason trueInfo1-negative-polarity"))
 
 	// test GetMessage
 	g.Expect(GetMessage(obj, "nil1")).To(Equal(""))
 	g.Expect(GetMessage(obj, "falseInfo1")).To(Equal("message falseInfo1"))
-	g.Expect(GetMessage(obj, "negative-trueInfo1")).To(Equal("message negative-trueInfo1"))
+	g.Expect(GetMessage(obj, "trueInfo1-negative-polarity")).To(Equal("message trueInfo1-negative-polarity"))
 
 	// test GetSeverity
 	expectedSeverity := clusterv1.ConditionSeverityInfo
 	g.Expect(GetSeverity(obj, "nil1")).To(BeNil())
 	severity := GetSeverity(obj, "falseInfo1")
 	g.Expect(severity).To(Equal(&expectedSeverity))
-	severity = GetSeverity(obj, "negative-trueInfo1")
+	severity = GetSeverity(obj, "trueInfo1-negative-polarity")
 	g.Expect(severity).To(Equal(&expectedSeverity))
 
 	// test GetLastTransitionTime
@@ -153,8 +153,8 @@ func TestSummary(t *testing.T) {
 	foo := TrueCondition("foo")
 	bar := FalseCondition("bar", "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1")
 	baz := FalseCondition("baz", "reason falseInfo2", clusterv1.ConditionSeverityInfo, "message falseInfo2")
-	negativeFoo := PositiveFalseCondition("negative-foo")
-	negativeBar := NegativeTrueCondition("negative-bar", "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1")
+	fooWithNegativePolarity := FalseConditionWithNegativePolarity("foo-negative-polarity")
+	barWithNegativePolarity := TrueConditionWithNegativePolarity("bar-negative-polarity", "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity")
 	existingReady := FalseCondition(clusterv1.ReadyCondition, "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1") // NB. existing ready has higher priority than other conditions
 
 	tests := []struct {
@@ -175,15 +175,15 @@ func TestSummary(t *testing.T) {
 		},
 		{
 			name:    "Returns ready condition with the summary of existing conditions with negative polarity (with default options)",
-			from:    getterWithConditions(negativeFoo, negativeBar),
-			options: []MergeOption{WithNegativePolarityConditions("negative-foo", "negative-bar")},
-			want:    FalseCondition(clusterv1.ReadyCondition, "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1"),
+			from:    getterWithConditions(fooWithNegativePolarity, barWithNegativePolarity),
+			options: []MergeOption{WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")},
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity"),
 		},
 		{
 			name:    "Returns ready condition with the summary of existing conditions with mixed polarity (with default options)",
-			from:    getterWithConditions(foo, bar, negativeFoo, negativeBar),
-			options: []MergeOption{WithNegativePolarityConditions("negative-foo", "negative-bar")},
-			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"), // bar take precedence on negativeBar because it is listed first
+			from:    getterWithConditions(foo, bar, fooWithNegativePolarity, barWithNegativePolarity),
+			options: []MergeOption{WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")},
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"), // bar take precedence on barWithNegativePolarity because it is listed first
 		},
 		{
 			name:    "Returns ready condition with the summary of existing conditions (using WithStepCounter options)",
@@ -223,15 +223,15 @@ func TestSummary(t *testing.T) {
 		},
 		{
 			name:    "Returns ready condition with the summary of selected conditions with negative polarity (using WithConditions options)",
-			from:    getterWithConditions(negativeFoo, negativeBar),
-			options: []MergeOption{WithConditions("negative-foo"), WithNegativePolarityConditions("negative-foo", "negative-bar")}, // negative-bar should be ignored
+			from:    getterWithConditions(fooWithNegativePolarity, barWithNegativePolarity),
+			options: []MergeOption{WithConditions("foo-negative-polarity"), WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")}, // bar-negative-polarity should be ignored because it is not listed in WithConditions
 			want:    TrueCondition(clusterv1.ReadyCondition),
 		},
 		{
 			name:    "Returns ready condition with the summary of selected conditions with mixed polarity (using WithConditions options)",
-			from:    getterWithConditions(foo, bar, negativeFoo, negativeBar),
-			options: []MergeOption{WithConditions("foo", "negative-foo", "negative-bar"), WithNegativePolarityConditions("negative-foo", "negative-bar")}, // bar should be ignored
-			want:    FalseCondition(clusterv1.ReadyCondition, "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1"),
+			from:    getterWithConditions(foo, bar, fooWithNegativePolarity, barWithNegativePolarity),
+			options: []MergeOption{WithConditions("foo", "foo-negative-polarity", "bar-negative-polarity"), WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")},
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity"),
 		},
 		{
 			name:    "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounter options)",

util/conditions/merge_strategies.go

@@ -52,6 +52,7 @@ func WithConditions(t ...clusterv1.ConditionType) MergeOption {
 }
 
 // WithNegativePolarityConditions instruct merge about which conditions should be considered having negative polarity.
+// IMPORTANT: this must be a subset of WithConditions.
 func WithNegativePolarityConditions(t ...clusterv1.ConditionType) MergeOption {
 	return func(c *mergeOptions) {
 		c.negativeConditionTypes = t

util/conditions/merge_test.go

@@ -86,10 +86,12 @@ func TestNewConditionsGroup(t *testing.T) {
 	t.Run("Negative polarity", func(t *testing.T) {
 		g := NewWithT(t)
 
-		conditions := []*clusterv1.Condition{nil1, positiveFalse1, positiveFalse1, negativeTrueInfo1, negativeTrueWarning1, negativeTrueWarning1, negativeTrueError1, negativeUnknown1}
+		conditions := []*clusterv1.Condition{nil1, false1WithNegativePolarity, false1WithNegativePolarity, trueInfo1WithNegativePolarity, trueWarning1WithNegativePolarity, trueWarning1WithNegativePolarity, trueError1WithNegativePolarity, unknown1WithNegativePolarity}
 
 		got := getConditionGroups(conditionsWithSource(&clusterv1.Cluster{}, conditions...))
 
+		// NOTE: groups always have a positive polarity
+
 		g.Expect(got).ToNot(BeNil())
 		g.Expect(got).To(HaveLen(5))
 
@@ -143,7 +145,7 @@ func TestNewConditionsGroup(t *testing.T) {
 	t.Run("Mixed polarity", func(t *testing.T) {
 		g := NewWithT(t)
 
-		conditions := []*clusterv1.Condition{nil1, true1, true1, falseInfo1, falseWarning1, falseWarning1, falseError1, unknown1, positiveFalse1, positiveFalse1, negativeTrueInfo1, negativeTrueWarning1, negativeTrueWarning1, negativeTrueError1, negativeUnknown1}
+		conditions := []*clusterv1.Condition{nil1, true1, true1, falseInfo1, falseWarning1, falseWarning1, falseError1, unknown1, false1WithNegativePolarity, false1WithNegativePolarity, trueInfo1WithNegativePolarity, trueWarning1WithNegativePolarity, trueWarning1WithNegativePolarity, trueError1WithNegativePolarity, unknown1WithNegativePolarity}
 
 		got := getConditionGroups(conditionsWithSource(&clusterv1.Cluster{}, conditions...))
 

util/conditions/setter.go

@@ -85,30 +85,30 @@ func TrueCondition(t clusterv1.ConditionType) *clusterv1.Condition {
 	}
 }
 
-// FalseCondition returns a condition with Status=False and the given type.
-func FalseCondition(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition {
+// TrueConditionWithNegativePolarity returns a condition with negative polarity, Status=True and the given type (Status=True has a negative meaning).
+func TrueConditionWithNegativePolarity(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition {
 	return &clusterv1.Condition{
 		Type:     t,
-		Status:   corev1.ConditionFalse,
+		Status:   corev1.ConditionTrue,
 		Reason:   reason,
 		Severity: severity,
 		Message:  fmt.Sprintf(messageFormat, messageArgs...),
 	}
 }
 
-// NegativeTrueCondition returns a negative polarity condition with Status=True and the given type (Status=True has a negative meaning).
-func NegativeTrueCondition(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition {
+// FalseCondition returns a condition with Status=False and the given type.
+func FalseCondition(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition {
 	return &clusterv1.Condition{
 		Type:     t,
-		Status:   corev1.ConditionTrue,
+		Status:   corev1.ConditionFalse,
 		Reason:   reason,
 		Severity: severity,
 		Message:  fmt.Sprintf(messageFormat, messageArgs...),
 	}
 }
 
-// PositiveFalseCondition returns a negative polarity condition with Status=false and the given type (Status=False has a positive meaning).
-func PositiveFalseCondition(t clusterv1.ConditionType) *clusterv1.Condition {
+// FalseConditionWithNegativePolarity returns a condition with negative polarity, Status=false and the given type (Status=False has a positive meaning).
+func FalseConditionWithNegativePolarity(t clusterv1.ConditionType) *clusterv1.Condition {
 	return &clusterv1.Condition{
 		Type:   t,
 		Status: corev1.ConditionFalse,
@@ -130,6 +130,11 @@ func MarkTrue(to Setter, t clusterv1.ConditionType) {
 	Set(to, TrueCondition(t))
 }
 
+// MarkTrueWithNegativePolarity sets Status=True for a condition with negative polarity and the given type (Status=True has a negative meaning).
+func MarkTrueWithNegativePolarity(to Setter, t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) {
+	Set(to, TrueConditionWithNegativePolarity(t, reason, severity, messageFormat, messageArgs...))
+}
+
 // MarkUnknown sets Status=Unknown for the condition with the given type.
 func MarkUnknown(to Setter, t clusterv1.ConditionType, reason, messageFormat string, messageArgs ...interface{}) {
 	Set(to, UnknownCondition(t, reason, messageFormat, messageArgs...))
@@ -140,14 +145,9 @@ func MarkFalse(to Setter, t clusterv1.ConditionType, reason string, severity clu
 	Set(to, FalseCondition(t, reason, severity, messageFormat, messageArgs...))
 }
 
-// MarkNegativeTrue sets Status=True for the negative polarity condition with the given type (Status=True has a negative meaning).
-func MarkNegativeTrue(to Setter, t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) {
-	Set(to, NegativeTrueCondition(t, reason, severity, messageFormat, messageArgs...))
-}
-
-// MarkPositiveFalse sets Status=False for the negative polarity condition with the given type (Status=False has a positive meaning).
-func MarkPositiveFalse(to Setter, t clusterv1.ConditionType) {
-	Set(to, PositiveFalseCondition(t))
+// MarkFalseWithNegativePolarity sets Status=False for a condition with negative polarity and the given type (Status=False has a positive meaning).
+func MarkFalseWithNegativePolarity(to Setter, t clusterv1.ConditionType) {
+	Set(to, FalseConditionWithNegativePolarity(t))
 }
 
 // SetSummary sets a Ready condition with the summary of all the conditions existing

util/conditions/setter_test.go

@@ -224,15 +224,15 @@ func TestMarkMethods(t *testing.T) {
 		Message: "messageBaz",
 	}))
 
-	// test MarkPositiveFalse
-	MarkPositiveFalse(cluster, "conditionFoo")
+	// test MarkFalseWithNegativePolarity
+	MarkFalseWithNegativePolarity(cluster, "conditionFoo")
 	g.Expect(Get(cluster, "conditionFoo")).To(HaveSameStateOf(&clusterv1.Condition{
 		Type:   "conditionFoo",
 		Status: corev1.ConditionFalse,
 	}))
 
-	// test MarkNegativeTrue
-	MarkNegativeTrue(cluster, "conditionBar", "reasonBar", clusterv1.ConditionSeverityError, "messageBar")
+	// test MarkTrueWithNegativePolarity
+	MarkTrueWithNegativePolarity(cluster, "conditionBar", "reasonBar", clusterv1.ConditionSeverityError, "messageBar")
 	g.Expect(Get(cluster, "conditionBar")).To(HaveSameStateOf(&clusterv1.Condition{
 		Type:     "conditionBar",
 		Status:   corev1.ConditionTrue,