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,27 @@ 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 HelmChart has not been processed"
+
+		// If the chart is not ready, we can likely provide a more
+		// concise reason why.
+		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 conditions.IsFalse(obj, meta.ReadyCondition):
+		return false, conditions.GetMessage(obj, meta.ReadyCondition)
+	case obj.Status.Artifact == nil:
+		return false, "does not have an artifact"
+	default:
+		return true, ""
+	}
+}

internal/controller/helmrelease_controller_test.go

@@ -2206,3 +2206,97 @@ 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 HelmChart has not been processed",
+		},
+		{
+			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 Ready=False",
+			obj: func() *sourcev1b2.HelmChart {
+				m := mock.DeepCopy()
+				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)
+			}
+		})
+	}
+}