Merge pull request #834 from fluxcd/propagate-hc-msg

controller: enrich "HelmChart not ready" messages

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)
+			}
+		})
+	}
+}