patch: use StalledCondition instead of custom condition type

Additional changes:
- resetReconciliationFailures is not exported anymore to prevent
  external sources calling it causing an inconsistent state of the
  resource.
- test: compare the ObservedGeneration of the condition after a wait.

References:
- #1061 (comment)
- #1061 (comment)
- #1061 (comment)

Signed-off-by: Balazs Nadasdi <[email protected]>

api/v1alpha2/terraform_types.go

@@ -529,8 +529,9 @@ const (
 
 // The potential reasons that are associated with condition types
 const (
-	ArtifactFailedReason            = "ArtifactFailed"
 	AccessDeniedReason              = "AccessDenied"
+	ArtifactFailedReason            = "ArtifactFailed"
+	RetryLimitReachedReason         = "RetryLimitReached"
 	DeletionBlockedByDependants     = "DeletionBlockedByDependantsReason"
 	DependencyNotReadyReason        = "DependencyNotReady"
 	DriftDetectedReason             = "DriftDetected"
@@ -556,12 +557,11 @@ const (
 
 // These constants are the Condition Types that the Terraform Resource works with
 const (
-	ConditionTypeApply         = "Apply"
-	ConditionTypeHealthCheck   = "HealthCheck"
-	ConditionTypeOutput        = "Output"
-	ConditionTypePlan          = "Plan"
-	ConditionTypeStateLocked   = "StateLocked"
-	ConditionRetryLimitReached = "RetryLimitReached"
+	ConditionTypeApply       = "Apply"
+	ConditionTypeHealthCheck = "HealthCheck"
+	ConditionTypeOutput      = "Output"
+	ConditionTypePlan        = "Plan"
+	ConditionTypeStateLocked = "StateLocked"
 )
 
 // Webhook stages
@@ -849,9 +849,9 @@ func TerraformStateLocked(terraform Terraform, lockID, message string) Terraform
 // indicating that the resource has reached its retry limit.
 func TerraformReachedLimit(terraform Terraform) Terraform {
 	newCondition := metav1.Condition{
-		Type:    ConditionRetryLimitReached,
+		Type:    meta.StalledCondition,
 		Status:  metav1.ConditionTrue,
-		Reason:  "ReachedHitRetryLimit",
+		Reason:  RetryLimitReachedReason,
 		Message: "Resource reached maximum number of retries.",
 	}
 	apimeta.SetStatusCondition(terraform.GetStatusConditions(), newCondition)
@@ -863,8 +863,8 @@ func TerraformReachedLimit(terraform Terraform) Terraform {
 // TerraformResetRetry will set a new condition on the Terraform resource
 // indicating that the resource retry count has been reset.
 func TerraformResetRetry(terraform Terraform) Terraform {
-	apimeta.RemoveStatusCondition(terraform.GetStatusConditions(), ConditionRetryLimitReached)
-	terraform.ResetReconciliationFailures()
+	apimeta.RemoveStatusCondition(terraform.GetStatusConditions(), meta.StalledCondition)
+	terraform.resetReconciliationFailures()
 
 	return terraform
 }
@@ -959,7 +959,7 @@ func (in *Terraform) IncrementReconciliationFailures() {
 	in.Status.ReconciliationFailures++
 }
 
-func (in *Terraform) ResetReconciliationFailures() {
+func (in *Terraform) resetReconciliationFailures() {
 	in.Status.ReconciliationFailures = 0
 }
 

controllers/tc000243_remediation_retry_test.go

@@ -8,6 +8,7 @@ import (
 
 	. "github.com/onsi/gomega"
 
+	"github.com/fluxcd/pkg/apis/meta"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1"
 	infrav1 "github.com/weaveworks/tf-controller/api/v1alpha2"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -128,8 +129,8 @@ spec:
 		Retries int64
 	}
 	expected := failedStatusCheckResult{
-		Type:    infrav1.ConditionRetryLimitReached,
-		Reason:  "ReachedHitRetryLimit",
+		Type:    meta.StalledCondition,
+		Reason:  infrav1.RetryLimitReachedReason,
 		Message: "Resource reached maximum number of retries.",
 		Status:  metav1.ConditionTrue,
 	}
@@ -154,18 +155,31 @@ spec:
 
 	// After 15s, it's still 3 and didn't go higher.
 	time.Sleep(15 * time.Second)
+
+	recheckHelloWorldTF := infrav1.Terraform{}
 	g.Eventually(func() interface{} {
-		err := k8sClient.Get(ctx, helloWorldTFKey, &createdHelloWorldTF)
-		if err != nil {
-			return nil
+		return k8sClient.Get(ctx, helloWorldTFKey, &recheckHelloWorldTF)
+	}, timeout, interval).Should(Succeed())
+
+	var retryCondition *metav1.Condition
+	for _, cond := range recheckHelloWorldTF.Status.Conditions {
+		if cond.Type == meta.StalledCondition && cond.Reason == infrav1.RetryLimitReachedReason {
+			retryCondition = &cond
+			break
 		}
-		for _, c := range createdHelloWorldTF.Status.Conditions {
-			if c.Type == "Ready" {
-				return createdHelloWorldTF.Status.ReconciliationFailures
-			}
+	}
+	var originalRetryCondition *metav1.Condition
+	for _, cond := range createdHelloWorldTF.Status.Conditions {
+		if cond.Type == meta.StalledCondition && cond.Reason == infrav1.RetryLimitReachedReason {
+			originalRetryCondition = &cond
+			break
 		}
-		return createdHelloWorldTF.Status
-	}, timeout, interval).Should(Not(BeNumerically(">", createdHelloWorldTF.Spec.Remediation.Retries)))
+	}
+
+	g.Expect(retryCondition).ToNot(BeNil(), "Terraform resource should have retry limit reached status condition")
+	g.Expect(originalRetryCondition).ToNot(BeNil(), "Terraform resource should have retry limit reached status condition")
+	g.Expect(recheckHelloWorldTF.Status.ReconciliationFailures).To(Equal(createdHelloWorldTF.Status.ReconciliationFailures))
+	g.Expect(retryCondition.ObservedGeneration).To(Equal(originalRetryCondition.ObservedGeneration))
 
 	It("should restart retry count")
 	By("when resource was updated")
@@ -176,8 +190,8 @@ spec:
 	g.Expect(k8sClient.Update(ctx, &createdHelloWorldTF)).Should(Succeed())
 
 	expected = failedStatusCheckResult{
-		Type:    infrav1.ConditionRetryLimitReached,
-		Reason:  "ReachedHitRetryLimit",
+		Type:    meta.StalledCondition,
+		Reason:  infrav1.RetryLimitReachedReason,
 		Message: "Resource reached maximum number of retries.",
 		Status:  metav1.ConditionTrue,
 		Retries: 2,
@@ -188,7 +202,7 @@ spec:
 			return nil
 		}
 		for _, c := range createdHelloWorldTF.Status.Conditions {
-			if c.Type == expected.Type {
+			if c.Type == expected.Type && c.Reason == expected.Reason {
 				return failedStatusCheckResult{
 					Type:    c.Type,
 					Reason:  c.Reason,