chore: address feedback

Signed-off-by: Robin Breathe <robin@isometry.net>

docs/spec/v1beta2/helmcharts.md

@@ -208,9 +208,9 @@ Values files also affect the generated artifact revision, see
 values files should be ignored rather than be considered errors.  It defaults to
 `false`.
 
-When `.spec.valuesFiles` is specified, the `.status.observedValuesFiles` field
+When `.spec.valuesFiles` and  `.spec.ignoreMissingValuesFiles` are specified,
-is populated with the list of values files that were found and actually
+the `.status.observedValuesFiles` field is populated with the list of values
-contributed to the packaged chart.
+files that were found and actually contributed to the packaged chart.
 
 ### Reconcile strategy
 

internal/controller/helmchart_controller.go

@@ -666,7 +666,6 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	cb := chart.NewRemoteBuilder(chartRepo)
 	opts := chart.BuildOptions{
 		ValuesFiles:              obj.GetValuesFiles(),
-		CachedChartValuesFiles:   obj.Status.ObservedValuesFiles,
 		IgnoreMissingValuesFiles: obj.Spec.IgnoreMissingValuesFiles,
 		Force:                    obj.Generation != obj.Status.ObservedGeneration,
 		// The remote builder will not attempt to download the chart if
@@ -676,6 +675,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	}
 	if artifact := obj.GetArtifact(); artifact != nil {
 		opts.CachedChart = r.Storage.LocalPath(*artifact)
+		opts.CachedChartValuesFiles = obj.Status.ObservedValuesFiles
 	}
 
 	// Set the VersionMetadata to the object's Generation if ValuesFiles is defined
@@ -888,7 +888,11 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, _ *patch.Se
 	// Record it on the object
 	obj.Status.Artifact = artifact.DeepCopy()
 	obj.Status.ObservedChartName = b.Name
+	if obj.Spec.IgnoreMissingValuesFiles {
 		obj.Status.ObservedValuesFiles = b.ValuesFiles
+	} else {
+		obj.Status.ObservedValuesFiles = nil
+	}
 
 	// Update symlink on a "best effort" basis
 	symURL, err := r.Storage.Symlink(artifact, "latest.tar.gz")

internal/controller/helmchart_controller_test.go

@@ -1661,7 +1661,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
 				t.Expect(obj.GetArtifact().Revision).To(Equal("0.1.0"))
 				t.Expect(obj.Status.URL).ToNot(BeEmpty())
 				t.Expect(obj.Status.ObservedChartName).To(Equal("helmchart"))
-				t.Expect(obj.Status.ObservedValuesFiles).To(BeEmpty())
+				t.Expect(obj.Status.ObservedValuesFiles).To(BeNil())
 			},
 			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
@@ -1684,7 +1684,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
 			afterFunc: func(t *WithT, obj *helmv1.HelmChart) {
 				t.Expect(obj.Status.Artifact.Path).To(Equal("testdata/charts/helmchart-0.1.0.tgz"))
 				t.Expect(obj.Status.ObservedChartName).To(BeEmpty())
-				t.Expect(obj.Status.ObservedValuesFiles).To(BeEmpty())
+				t.Expect(obj.Status.ObservedValuesFiles).To(BeNil())
 				t.Expect(obj.Status.URL).To(BeEmpty())
 			},
 		},
@@ -1724,7 +1724,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
 				t.Expect(obj.GetArtifact().Revision).To(Equal("0.1.0"))
 				t.Expect(obj.Status.URL).ToNot(BeEmpty())
 				t.Expect(obj.Status.ObservedChartName).To(Equal("helmchart"))
-				t.Expect(obj.Status.ObservedValuesFiles).To(BeEmpty())
+				t.Expect(obj.Status.ObservedValuesFiles).To(BeNil())
 			},
 			want: sreconcile.ResultSuccess,
 			assertConditions: []metav1.Condition{
@@ -1754,6 +1754,27 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
 			beforeFunc: func(obj *helmv1.HelmChart) {
 				conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "")
 			},
+			afterFunc: func(t *WithT, obj *helmv1.HelmChart) {
+				t.Expect(obj.GetArtifact()).ToNot(BeNil())
+				t.Expect(obj.GetArtifact().Digest).To(Equal("sha256:bbdf96023c912c393b49d5238e227576ed0d20d1bb145d7476d817b80e20c11a"))
+				t.Expect(obj.GetArtifact().Revision).To(Equal("0.1.0"))
+				t.Expect(obj.Status.URL).ToNot(BeEmpty())
+				t.Expect(obj.Status.ObservedChartName).To(Equal("helmchart"))
+				t.Expect(obj.Status.ObservedValuesFiles).To(BeNil())
+			},
+			want: sreconcile.ResultSuccess,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, helmv1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"),
+			},
+		},
+		{
+			name:  "Updates ObservedValuesFiles with IgnoreMissingValuesFiles after creating new artifact",
+			build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", []string{"values.yaml", "override.yaml"}),
+			beforeFunc: func(obj *helmv1.HelmChart) {
+				conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "")
+				obj.Spec.ValuesFiles = []string{"values.yaml", "missing.yaml", "override.yaml"}
+				obj.Spec.IgnoreMissingValuesFiles = true
+			},
 			afterFunc: func(t *WithT, obj *helmv1.HelmChart) {
 				t.Expect(obj.GetArtifact()).ToNot(BeNil())
 				t.Expect(obj.GetArtifact().Digest).To(Equal("sha256:bbdf96023c912c393b49d5238e227576ed0d20d1bb145d7476d817b80e20c11a"))

internal/helm/chart/builder_local.go

@@ -120,7 +120,12 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string,
 			if err = curMeta.Validate(); err == nil {
 				if result.Name == curMeta.Name && result.Version == curMeta.Version {
 					result.Path = opts.CachedChart
+					result.ValuesFiles = opts.GetValuesFiles()
+					if opts.CachedChartValuesFiles != nil {
+						// If the cached chart values files are set, we should use them
+						// instead of reporting the values files.
 						result.ValuesFiles = opts.CachedChartValuesFiles
+					}
 					result.Packaged = requiresPackaging
 
 					return result, nil

internal/helm/chart/builder_remote.go

@@ -202,7 +202,12 @@ func generateBuildResult(cv *repo.ChartVersion, opts BuildOptions) (*Build, bool
 			if err = curMeta.Validate(); err == nil {
 				if result.Name == curMeta.Name && result.Version == curMeta.Version {
 					result.Path = opts.CachedChart
+					result.ValuesFiles = opts.GetValuesFiles()
+					if opts.CachedChartValuesFiles != nil {
+						// If the cached chart values files are set, we should use them
+						// instead of reporting the values files.
 						result.ValuesFiles = opts.CachedChartValuesFiles
+					}
 					result.Packaged = requiresPackaging
 					return result, true, nil
 				}