Address review comments

- Use `Unknown` status for the `TestSuccess` condition when tests
  have not been run yet.
- Update Ready summarization logic to incorportate conditions with an
  Unknown status. Within the context of readiness, this always caises
  Ready=False when the condition is included in the summarization.
- Variety of tiny fixes.
- Tiny nits in test mocks to prevent confusion.

Signed-off-by: Hidde Beydals <[email protected]>

internal/action/verify.go

@@ -43,7 +43,7 @@ var (
 
 // ReleaseTargetChanged returns true if the given release and/or chart
 // name have been mutated in such a way that it no longer has the same release
-// target as the Status.Current. By comparing the (storage) namespace, and
+// target as the Status.Current, by comparing the (storage) namespace, and
 // release and chart names. This can be used to e.g. trigger a garbage
 // collection of the old release before installing the new one.
 func ReleaseTargetChanged(obj *v2.HelmRelease, chartName string) bool {
@@ -126,9 +126,9 @@ func VerifyLastStorageItem(config *helmaction.Configuration, info *v2.HelmReleas
 }
 
 // VerifyReleaseObject verifies the data of the given v2beta2.HelmReleaseInfo
-// matches the given Helm release object. It returns the verified
-// release, or an error of type ErrReleaseDigest or ErrReleaseNotObserved
-// indicating the reason for the verification failure.
+// matches the given Helm release object. It returns an error of type
+// ErrReleaseDigest or ErrReleaseNotObserved indicating the reason for the
+// verification failure, or nil.
 func VerifyReleaseObject(info *v2.HelmReleaseInfo, rls *helmrelease.Release) error {
 	relDig, err := digest.Parse(info.Digest)
 	if err != nil {

internal/reconcile/action.go

@@ -80,7 +80,7 @@ func NextAction(ctx context.Context, cfg *action.ConfigFactory, recorder record.
 	// unexpectedly. Unlock the release and e.g. retry again.
 	if rls.Info.Status.IsPending() {
 		log.Info("observed release is in stale pending state")
-		return &Unlock{configFactory: cfg}, nil
+		return NewUnlock(cfg, recorder), nil
 	}
 
 	remediation := req.Object.GetActiveRemediation()
@@ -110,9 +110,7 @@ func NextAction(ctx context.Context, cfg *action.ConfigFactory, recorder record.
 		// Confirm the current release matches the desired config.
 		if err = action.VerifyRelease(rls, cur, req.Chart.Metadata, req.Values); err != nil {
 			switch err {
-			case action.ErrChartChanged:
-				return NewUpgrade(cfg, recorder), nil
-			case action.ErrConfigDigest:
+			case action.ErrChartChanged, action.ErrConfigDigest:
 				return NewUpgrade(cfg, recorder), nil
 			default:
 				// Error out on any other error as we cannot determine what

internal/reconcile/atomic_release_test.go

@@ -146,6 +146,10 @@ func TestAtomicRelease_Reconcile(t *testing.T) {
 		)
 		g.Expect(err).ToNot(HaveOccurred())
 
+		// We use a fake client here to allow us to work with a minimal release
+		// object mock. As the fake client does not perform any validation.
+		// However, for the Helm storage driver to work, we need a real client
+		// which is therefore initialized separately above.
 		client := fake.NewClientBuilder().
 			WithScheme(testEnv.Scheme()).
 			WithObjects(obj).

internal/reconcile/install.go

@@ -154,7 +154,8 @@ func (r *Install) success(req *Request) {
 	// Mark install success on object.
 	conditions.MarkTrue(req.Object, v2.ReleasedCondition, v2.InstallSucceededReason, msg)
 	if req.Object.GetTest().Enable && !cur.HasBeenTested() {
-		conditions.MarkFalse(req.Object, v2.TestSuccessCondition, "Pending", fmtTestPending, cur.FullReleaseName(), cur.VersionedChartName())
+		conditions.MarkUnknown(req.Object, v2.TestSuccessCondition, "Pending", fmtTestPending,
+			cur.FullReleaseName(), cur.VersionedChartName())
 	}
 
 	// Record event.

internal/reconcile/release.go

@@ -123,6 +123,11 @@ func summarize(req *Request) {
 	})
 
 	status := conds[0].Status
+	// Unknown is considered False within the context of Readiness.
+	if status == metav1.ConditionUnknown {
+		status = metav1.ConditionFalse
+	}
+
 	// Any remediated state is considered an error.
 	if conds[0].Type == v2.RemediatedCondition {
 		status = metav1.ConditionFalse

internal/reconcile/release_test.go

@@ -293,6 +293,53 @@ func Test_summarize(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "with test hooks enabled and pending tests",
+			conditions: []metav1.Condition{
+				{
+					Type:               v2.ReleasedCondition,
+					Status:             metav1.ConditionTrue,
+					Reason:             v2.InstallSucceededReason,
+					Message:            "Install complete",
+					ObservedGeneration: 1,
+				},
+				{
+					Type:               v2.TestSuccessCondition,
+					Status:             metav1.ConditionUnknown,
+					Reason:             "Pending",
+					Message:            "Release is awaiting tests",
+					ObservedGeneration: 1,
+				},
+			},
+			spec: &v2.HelmReleaseSpec{
+				Test: &v2.Test{
+					Enable: true,
+				},
+			},
+			expect: []metav1.Condition{
+				{
+					Type:               meta.ReadyCondition,
+					Status:             metav1.ConditionFalse,
+					Reason:             "Pending",
+					Message:            "Release is awaiting tests",
+					ObservedGeneration: 1,
+				},
+				{
+					Type:               v2.ReleasedCondition,
+					Status:             metav1.ConditionTrue,
+					Reason:             v2.InstallSucceededReason,
+					Message:            "Install complete",
+					ObservedGeneration: 1,
+				},
+				{
+					Type:               v2.TestSuccessCondition,
+					Status:             metav1.ConditionUnknown,
+					Reason:             "Pending",
+					Message:            "Release is awaiting tests",
+					ObservedGeneration: 1,
+				},
+			},
+		},
 		{
 			name:       "with remediation failure",
 			generation: 1,
@@ -470,7 +517,7 @@ func Test_summarize(t *testing.T) {
 					Status:             metav1.ConditionTrue,
 					Reason:             v2.UpgradeSucceededReason,
 					Message:            "Upgrade finished",
-					ObservedGeneration: 6,
+					ObservedGeneration: 5,
 				},
 				{
 					Type:               v2.ReleasedCondition,
@@ -619,7 +666,7 @@ func Test_conditionallyDeleteRemediated(t *testing.T) {
 			name: "Released=False",
 			conditions: []metav1.Condition{
 				*conditions.TrueCondition(v2.RemediatedCondition, v2.RollbackSucceededReason, "Rollback finished"),
-				*conditions.FalseCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "Upgrade finished"),
+				*conditions.FalseCondition(v2.ReleasedCondition, v2.UpgradeFailedReason, "Upgrade failed"),
 			},
 			expectDelete: false,
 		},
@@ -662,7 +709,7 @@ func Test_conditionallyDeleteRemediated(t *testing.T) {
 			conditions: []metav1.Condition{
 				*conditions.TrueCondition(v2.RemediatedCondition, v2.RollbackSucceededReason, "Rollback finished"),
 				*conditions.TrueCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "Upgrade finished"),
-				*conditions.FalseCondition(v2.TestSuccessCondition, v2.TestSucceededReason, "Test hooks succeeded"),
+				*conditions.FalseCondition(v2.TestSuccessCondition, v2.TestFailedReason, "Test hooks failed"),
 			},
 			expectDelete: true,
 		},

internal/reconcile/upgrade.go

@@ -153,7 +153,7 @@ func (r *Upgrade) success(req *Request) {
 	// Mark upgrade success on object.
 	conditions.MarkTrue(req.Object, v2.ReleasedCondition, v2.UpgradeSucceededReason, msg)
 	if req.Object.GetTest().Enable && !cur.HasBeenTested() {
-		conditions.MarkFalse(req.Object, v2.TestSuccessCondition, "Pending", fmtTestPending,
+		conditions.MarkUnknown(req.Object, v2.TestSuccessCondition, "Pending", fmtTestPending,
 			cur.FullReleaseName(), cur.VersionedChartName())
 	}