From 9b57d3bc52426ffd75ff0457e7155300e9b478ce Mon Sep 17 00:00:00 2001 From: Robin Breathe Date: Wed, 1 May 2024 17:23:54 +0200 Subject: [PATCH] chore: update tests Signed-off-by: Robin Breathe --- api/v1beta2/helmchart_types.go | 4 +- .../source.toolkit.fluxcd.io_helmcharts.yaml | 5 +- docs/api/v1beta2/source.md | 4 +- internal/controller/helmchart_controller.go | 6 +- .../controller/helmchart_controller_test.go | 123 +++++++++++++++++- internal/helm/chart/builder.go | 9 +- internal/helm/chart/builder_local.go | 4 +- internal/helm/chart/builder_remote.go | 2 +- 8 files changed, 137 insertions(+), 20 deletions(-) diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index eb46fefc..417a5a50 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -147,7 +147,9 @@ type HelmChartStatus struct { // +optional ObservedChartName string `json:"observedChartName,omitempty"` - // ObservedValuesFiles are the last observed value files. + // ObservedValuesFiles are the observed value files of the last successful + // reconciliation. + // It matches the chart in the last successfully reconciled artifact. // +optional ObservedValuesFiles []string `json:"observedValuesFiles,omitempty"` diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml index e35b480c..0a0a9a2a 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml @@ -644,7 +644,10 @@ spec: of the HelmChartSpec.SourceRef. type: string observedValuesFiles: - description: ObservedValuesFiles are the last observed value files. + description: |- + ObservedValuesFiles are the observed value files of the last successful + reconciliation. + It matches the chart in the last successfully reconciled artifact. items: type: string type: array diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md index 2e42e560..7144c856 100644 --- a/docs/api/v1beta2/source.md +++ b/docs/api/v1beta2/source.md @@ -2469,7 +2469,9 @@ resolved chart reference.

(Optional) -

ObservedValuesFiles are the last observed value files.

+

ObservedValuesFiles are the observed value files of the last successful +reconciliation. +It matches the chart in the last successfully reconciled artifact.

diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index a0cf8e15..547ccac4 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -666,7 +666,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * cb := chart.NewRemoteBuilder(chartRepo) opts := chart.BuildOptions{ ValuesFiles: obj.GetValuesFiles(), - ObservedValuesFiles: obj.Status.ObservedValuesFiles, + 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 @@ -763,12 +763,12 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // Configure builder options, including any previously cached chart opts := chart.BuildOptions{ ValuesFiles: obj.GetValuesFiles(), - ObservedValuesFiles: obj.Status.ObservedValuesFiles, IgnoreMissingValuesFiles: obj.Spec.IgnoreMissingValuesFiles, Force: obj.Generation != obj.Status.ObservedGeneration, } - if artifact := obj.Status.Artifact; artifact != nil { + if artifact := obj.GetArtifact(); artifact != nil { opts.CachedChart = r.Storage.LocalPath(*artifact) + opts.CachedChartValuesFiles = obj.Status.ObservedValuesFiles } // Configure revision metadata for chart build if we should react to revision changes diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index c8ec1cef..2c06955c 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -927,6 +927,23 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) { g.Expect(build.Path).To(BeARegularFile()) }, }, + { + name: "Uses artifact as build cache with observedValuesFiles", + beforeFunc: func(obj *helmv1.HelmChart, repository *helmv1.HelmRepository) { + obj.Spec.Chart = chartName + obj.Spec.Version = chartVersion + obj.Status.Artifact = &sourcev1.Artifact{Path: chartName + "-" + chartVersion + ".tgz"} + obj.Status.ObservedValuesFiles = []string{"values.yaml", "override.yaml"} + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, obj *helmv1.HelmChart, build chart.Build) { + g.Expect(build.Name).To(Equal(chartName)) + g.Expect(build.Version).To(Equal(chartVersion)) + g.Expect(build.Path).To(Equal(filepath.Join(serverFactory.Root(), obj.Status.Artifact.Path))) + g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.ValuesFiles).To(Equal([]string{"values.yaml", "override.yaml"})) + }, + }, { name: "Sets Generation as VersionMetadata with values files", beforeFunc: func(obj *helmv1.HelmChart, repository *helmv1.HelmRepository) { @@ -940,6 +957,51 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) { g.Expect(build.Version).To(Equal(higherChartVersion + "+3")) g.Expect(build.Path).ToNot(BeEmpty()) g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.ValuesFiles).To(Equal([]string{"values.yaml", "override.yaml"})) + }, + cleanFunc: func(g *WithT, build *chart.Build) { + g.Expect(os.Remove(build.Path)).To(Succeed()) + }, + }, + { + name: "Missing values files are an error", + beforeFunc: func(obj *helmv1.HelmChart, repository *helmv1.HelmRepository) { + obj.Spec.Chart = chartName + obj.Spec.ValuesFiles = []string{"missing.yaml"} + }, + wantErr: &chart.BuildError{Err: errors.New("values files merge error: failed to merge chart values: no values file found at path 'missing.yaml'")}, + }, + { + name: "All missing values files ignored", + beforeFunc: func(obj *helmv1.HelmChart, repository *helmv1.HelmRepository) { + obj.Spec.Chart = chartName + obj.Spec.Version = chartVersion + obj.Spec.ValuesFiles = []string{"missing.yaml"} + obj.Spec.IgnoreMissingValuesFiles = true + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, obj *helmv1.HelmChart, build chart.Build) { + g.Expect(build.Name).To(Equal(chartName)) + g.Expect(build.Version).To(Equal(chartVersion + "+0")) + g.Expect(build.ValuesFiles).To(BeEmpty()) + }, + cleanFunc: func(g *WithT, build *chart.Build) { + g.Expect(os.Remove(build.Path)).To(Succeed()) + }, + }, + { + name: "Partial missing values files ignored", + beforeFunc: func(obj *helmv1.HelmChart, repository *helmv1.HelmRepository) { + obj.Spec.Chart = chartName + obj.Spec.Version = chartVersion + obj.Spec.ValuesFiles = []string{"values.yaml", "override.yaml", "invalid.yaml"} + obj.Spec.IgnoreMissingValuesFiles = true + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, obj *helmv1.HelmChart, build chart.Build) { + g.Expect(build.Name).To(Equal(chartName)) + g.Expect(build.Version).To(Equal(chartVersion + "+0")) + g.Expect(build.ValuesFiles).To(Equal([]string{"values.yaml", "override.yaml"})) }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) @@ -1211,6 +1273,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { g.Expect(build.Version).To(Equal(metadata.Version)) g.Expect(build.Path).To(Equal(storage.LocalPath(*cachedArtifact.DeepCopy()))) g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.ValuesFiles).To(BeEmpty()) }, }, { @@ -1433,6 +1496,10 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { g.Expect(build.Version).To(Equal("0.1.0+3")) g.Expect(build.ResolvedDependencies).To(Equal(0)) g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.ValuesFiles).To(Equal([]string{ + "testdata/charts/helmchart/values.yaml", + "testdata/charts/helmchart/override.yaml", + })) }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) @@ -1451,6 +1518,24 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { g.Expect(build.Version).To(Equal("0.1.0")) g.Expect(build.Path).To(Equal(storage.LocalPath(*cachedArtifact.DeepCopy()))) g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.ValuesFiles).To(BeEmpty()) + }, + }, + { + name: "Chart from storage cache with ObservedValuesFiles", + source: *chartsArtifact.DeepCopy(), + beforeFunc: func(obj *helmv1.HelmChart) { + obj.Spec.Chart = "testdata/charts/helmchart-0.1.0.tgz" + obj.Status.Artifact = cachedArtifact.DeepCopy() + obj.Status.ObservedValuesFiles = []string{"values.yaml", "override.yaml"} + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, build chart.Build) { + g.Expect(build.Name).To(Equal("helmchart")) + g.Expect(build.Version).To(Equal("0.1.0")) + g.Expect(build.Path).To(Equal(storage.LocalPath(*cachedArtifact.DeepCopy()))) + g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.ValuesFiles).To(Equal([]string{"values.yaml", "override.yaml"})) }, }, { @@ -1468,6 +1553,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) { g.Expect(build.Version).To(Equal("0.1.0")) g.Expect(build.Path).ToNot(Equal(storage.LocalPath(*cachedArtifact.DeepCopy()))) g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.ValuesFiles).To(BeEmpty()) }, cleanFunc: func(g *WithT, build *chart.Build) { g.Expect(os.Remove(build.Path)).To(Succeed()) @@ -1565,7 +1651,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, { name: "Copying artifact to storage from build makes ArtifactInStorage=True", - build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz"), + build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", nil), beforeFunc: func(obj *helmv1.HelmChart) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") }, @@ -1575,6 +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()) }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ @@ -1597,6 +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.URL).To(BeEmpty()) }, }, @@ -1626,7 +1714,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, { name: "Removes ArtifactOutdatedCondition after creating new artifact", - build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz"), + build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", nil), beforeFunc: func(obj *helmv1.HelmChart) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") }, @@ -1636,6 +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()) }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ @@ -1644,7 +1733,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, { name: "Creates latest symlink to the created artifact", - build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz"), + build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", nil), afterFunc: func(t *WithT, obj *helmv1.HelmChart) { t.Expect(obj.GetArtifact()).ToNot(BeNil()) @@ -1659,6 +1748,25 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, helmv1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), }, }, + { + name: "Updates ObservedValuesFiles 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", "") + }, + 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(Equal([]string{"values.yaml", "override.yaml"})) + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, helmv1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), + }, + }, } for _, tt := range tests { @@ -2016,7 +2124,7 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { } } -func mockChartBuild(name, version, path string) *chart.Build { +func mockChartBuild(name, version, path string, valuesFiles []string) *chart.Build { var copyP string if path != "" { f, err := os.Open(path) @@ -2032,9 +2140,10 @@ func mockChartBuild(name, version, path string) *chart.Build { } } return &chart.Build{ - Name: name, - Version: version, - Path: copyP, + Name: name, + Version: version, + Path: copyP, + ValuesFiles: valuesFiles, } } diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go index 98725d3e..b56c8c9a 100644 --- a/internal/helm/chart/builder.go +++ b/internal/helm/chart/builder.go @@ -107,10 +107,11 @@ type BuildOptions struct { // ValuesFiles can be set to a list of relative paths, used to compose // and overwrite an alternative default "values.yaml" for the chart. ValuesFiles []string - // ObservedValuesFiles is calculated when the chart is built. If BuildOptions.IgnoreMissingValuesFiles is set, - // this list will contain the values files that were actually found on disk. - ObservedValuesFiles []string - // IgnoreMissingValuesFiles controls whether to silently ignore missing values files rather than failing. + // CachedChartValuesFiles is a list of relative paths that were used to + // build the cached chart. + CachedChartValuesFiles []string + // IgnoreMissingValuesFiles controls whether to silently ignore missing + // values files rather than failing. IgnoreMissingValuesFiles bool // CachedChart can be set to the absolute path of a chart stored on // the local filesystem, and is used for simple validation by metadata diff --git a/internal/helm/chart/builder_local.go b/internal/helm/chart/builder_local.go index cee286a3..23e433cc 100644 --- a/internal/helm/chart/builder_local.go +++ b/internal/helm/chart/builder_local.go @@ -120,7 +120,7 @@ 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.ObservedValuesFiles + result.ValuesFiles = opts.CachedChartValuesFiles result.Packaged = requiresPackaging return result, nil @@ -191,7 +191,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // mergeFileValues merges the given value file paths into a single "values.yaml" map. // The provided (relative) paths may not traverse outside baseDir. By default, a missing -// file is considered an error. If ignoreMissing is set true, missing files are ignored. +// file is considered an error. If ignoreMissing is true, missing files are ignored. // It returns the merge result and the list of files that contributed to that result, // or an error. func mergeFileValues(baseDir string, paths []string, ignoreMissing bool) (map[string]interface{}, []string, error) { diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 222f0a1b..e076680b 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -202,7 +202,7 @@ 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.ObservedValuesFiles + result.ValuesFiles = opts.CachedChartValuesFiles result.Packaged = requiresPackaging return result, true, nil }