controller: enrich "HelmChart not ready" messages

This propagates the reason a HelmChart is (likely) not ready to the
message of the Ready condition.

The goal of this is to make it easier for people to reason about a
potential failure that may be happening while retrieving the chart,
without having to inspect the HelmChart itself.

As at times, they may not have access (due to e.g. not being able to
access the namespace, while the controller is allowed to create the
object there), or are simply not aware of the fact that this object
is created by the controller for them.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Author: hiddeco

internal/controller/helmrelease_controller.go

@@ -276,9 +276,9 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 		return ctrl.Result{}, err
 	}
 
-	// Check chart readiness.
-	if hc.Generation != hc.Status.ObservedGeneration || !conditions.IsReady(hc) || hc.GetArtifact() == nil {
-		msg := fmt.Sprintf("HelmChart '%s/%s' is not ready", hc.GetNamespace(), hc.GetName())
+	// Check if the HelmChart is ready.
+	if ready, reason := isHelmChartReady(hc); !ready {
+		msg := fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", hc.GetNamespace(), hc.GetName(), reason)
 		log.Info(msg)
 		conditions.MarkFalse(obj, meta.ReadyCondition, "HelmChartNotReady", msg)
 		// Do not requeue immediately, when the artifact is created
@@ -703,3 +703,29 @@ func (r *HelmReleaseReconciler) requestsForHelmChartChange(ctx context.Context,
 	}
 	return reqs
 }
+
+// isHelmChartReady returns true if the given HelmChart is ready, and a reason
+// why it is not ready otherwise.
+func isHelmChartReady(obj *sourcev1.HelmChart) (bool, string) {
+	switch {
+	case obj.Generation != obj.Status.ObservedGeneration:
+		msg := "latest generation of object has not been reconciled"
+
+		// If the chart is not ready, we can likely provide a more
+		// concise reason why.
+		// We do not do this while the Generation matches the Observed
+		// Generation, as we could then potentially stall on e.g.
+		// temporary errors which do not have an impact as long as
+		// there is an Artifact for the current Generation.
+		if conditions.IsFalse(obj, meta.ReadyCondition) {
+			msg = conditions.GetMessage(obj, meta.ReadyCondition)
+		}
+		return false, msg
+	case conditions.IsStalled(obj):
+		return false, conditions.GetMessage(obj, meta.StalledCondition)
+	case obj.Status.Artifact == nil:
+		return false, "does not have an artifact"
+	default:
+		return true, ""
+	}
+}

internal/controller/helmrelease_controller_test.go

@@ -179,7 +179,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		}))
 	})
 
-	t.Run("waits for HelmChart to be ready", func(t *testing.T) {
+	t.Run("waits for HelmChart to have an Artifact", func(t *testing.T) {
 		g := NewWithT(t)
 
 		chart := &sourcev1b2.HelmChart{
@@ -190,11 +190,11 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 			},
 			Status: sourcev1b2.HelmChartStatus{
 				ObservedGeneration: 2,
-				Artifact:           &sourcev1.Artifact{},
+				Artifact:           nil,
 				Conditions: []metav1.Condition{
 					{
 						Type:   meta.ReadyCondition,
-						Status: metav1.ConditionFalse,
+						Status: metav1.ConditionTrue,
 					},
 				},
 			},
@@ -2206,3 +2206,87 @@ func TestValuesReferenceValidation(t *testing.T) {
 		})
 	}
 }
+
+func Test_isHelmChartReady(t *testing.T) {
+	mock := &sourcev1b2.HelmChart{
+		ObjectMeta: metav1.ObjectMeta{
+			Generation: 2,
+		},
+		Status: sourcev1b2.HelmChartStatus{
+			ObservedGeneration: 2,
+			Conditions: []metav1.Condition{
+				{
+					Type:   meta.ReadyCondition,
+					Status: metav1.ConditionTrue,
+				},
+			},
+			Artifact: &sourcev1.Artifact{},
+		},
+	}
+
+	tests := []struct {
+		name       string
+		obj        *sourcev1b2.HelmChart
+		want       bool
+		wantReason string
+	}{
+		{
+			name: "chart is ready",
+			obj:  mock.DeepCopy(),
+			want: true,
+		},
+		{
+			name: "chart generation differs from observed generation while Ready=True",
+			obj: func() *sourcev1b2.HelmChart {
+				m := mock.DeepCopy()
+				m.Generation = 3
+				return m
+			}(),
+			want:       false,
+			wantReason: "latest generation of object has not been reconciled",
+		},
+		{
+			name: "chart generation differs from observed generation while Ready=False",
+			obj: func() *sourcev1b2.HelmChart {
+				m := mock.DeepCopy()
+				m.Generation = 3
+				conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason")
+				return m
+			}(),
+			want:       false,
+			wantReason: "some reason",
+		},
+		{
+			name: "chart has Stalled=True",
+			obj: func() *sourcev1b2.HelmChart {
+				m := mock.DeepCopy()
+				conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason")
+				conditions.MarkStalled(m, "Reason", "some stalled reason")
+				return m
+			}(),
+			want:       false,
+			wantReason: "some stalled reason",
+		},
+		{
+			name: "chart does not have an Artifact",
+			obj: func() *sourcev1b2.HelmChart {
+				m := mock.DeepCopy()
+				m.Status.Artifact = nil
+				return m
+			}(),
+			want:       false,
+			wantReason: "does not have an artifact",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, gotReason := isHelmChartReady(tt.obj)
+			if got != tt.want {
+				t.Errorf("isHelmChartReady() got = %v, want %v", got, tt.want)
+			}
+			if gotReason != tt.wantReason {
+				t.Errorf("isHelmChartReady() reason = %v, want %v", gotReason, tt.wantReason)
+			}
+		})
+	}
+}