diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 0df4d8dd7..82bebd6a1 100644 --- a/internal/controller/helmrelease_controller.go +++ b/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, "" + } +} diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index 1376770c4..75fa2149a 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/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) + } + }) + } +}