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