diff --git a/chartify.go b/chartify.go index bcf4efb..3422217 100644 --- a/chartify.go +++ b/chartify.go @@ -502,7 +502,11 @@ func (r *Runner) Chartify(release, dirOrChart string, opts ...ChartifyOption) (s } } - if needsKustomizeBuild { + // When the chart rendered no resources, there is nothing for kustomize to build or + // patch. Skip the kustomize step entirely so an empty render is treated as a no-op + // success even when JsonPatches/StrategicMergePatches/Transformers are configured + // (the patches simply have no resources to apply to). See issue #206. + if needsKustomizeBuild && len(generatedManifestFiles) > 0 { patchOpts := &PatchOpts{ JsonPatches: u.JsonPatches, StrategicMergePatches: u.StrategicMergePatches, diff --git a/chartify_test.go b/chartify_test.go index 1a559f2..eb53096 100644 --- a/chartify_test.go +++ b/chartify_test.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -348,3 +349,80 @@ func TestUseHelmChartsInKustomize(t *testing.T) { }) } } + +// TestEmptyRenderCleansChartDependencies verifies that when a chart renders no +// resources, the empty-render path still runs the Chart.yaml `dependencies` +// cleanup and lock-file removal. Without that cleanup, a chart that declares +// dependencies would leave Chart.yaml referencing subcharts whose charts/ +// directory was removed, causing a subsequent `helm template` to fail with +// "found in Chart.yaml, but missing in charts/ directory". +// +// The integration harness cannot detect this because doTest always runs +// `helm dependency build` on the output, which masks the missing-charts error; +// this test does not, so the failure mode is directly observable. +// See https://github.com/helmfile/chartify/issues/206 +func TestEmptyRenderCleansChartDependencies(t *testing.T) { + helmBin := "helm" + if h := os.Getenv("HELM_BIN"); h != "" { + helmBin = h + } + r := New(HelmBin(helmBin)) + if !(r.IsHelm3() || r.IsHelm4()) { + t.Skip("test requires helm 3 or 4 (dependencies are stored in Chart.yaml)") + } + + // Build a parent chart in a temp dir with a local file:// subchart dependency. + // Both parent and subchart render nothing (templates gated behind enabled=false), + // so `helm template --output-dir` produces an empty dir and ReplaceWithRendered + // takes the empty-render branch. + parentDir := t.TempDir() + subDir := filepath.Join(parentDir, "emptysub") + + writeFile := func(path, content string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0644)) + } + + // Subchart: renders nothing by default. + writeFile(filepath.Join(subDir, "Chart.yaml"), + "apiVersion: v2\nname: emptysub\ntype: application\nversion: 0.1.0\n") + writeFile(filepath.Join(subDir, "values.yaml"), "enabled: false\n") + writeFile(filepath.Join(subDir, "templates", "cm.yaml"), + "{{- if .Values.enabled }}\napiVersion: v1\nkind: ConfigMap\nmetadata:\n name: {{ .Release.Name }}-sub\n{{- end }}\n") + + // Parent: renders nothing by default, depends on the subchart above. + writeFile(filepath.Join(parentDir, "Chart.yaml"), + "apiVersion: v2\nname: emptydep\ntype: application\nversion: 0.1.0\n"+ + "dependencies:\n"+ + " - name: emptysub\n"+ + " repository: file://./emptysub\n"+ + " version: 0.1.0\n") + writeFile(filepath.Join(parentDir, "values.yaml"), + "enabled: false\nemptysub:\n enabled: false\n") + writeFile(filepath.Join(parentDir, "templates", "cm.yaml"), + "{{- if .Values.enabled }}\napiVersion: v1\nkind: ConfigMap\nmetadata:\n name: {{ .Release.Name }}-cm\n{{- end }}\n") + + // OverrideNamespace forces ReplaceWithRendered to run. + outDir, err := r.Chartify("myapp", parentDir, WithChartifyOpts(&ChartifyOpts{ + OverrideNamespace: "test-ns", + })) + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(outDir) }) + + // After chartify, Chart.yaml must no longer declare dependencies, otherwise a + // subsequent `helm template` would fail with "found in Chart.yaml, but missing + // in charts/ directory: emptysub". + chartYaml, err := os.ReadFile(filepath.Join(outDir, "Chart.yaml")) + require.NoError(t, err) + require.NotContainsf(t, string(chartYaml), "dependencies:", + "Chart.yaml dependencies field should have been removed after an empty render; got:\n%s", chartYaml) + + // Templating the chartified output must succeed and render nothing. NB: this does + // NOT run `helm dependency build` first, so any leftover Chart.yaml dependency + // would surface here as a "missing in charts/ directory" error. + cmd := exec.CommandContext(context.Background(), helmBin, "template", "myapp", outDir) + tmplOut, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "helm template on chartified output failed: %s", tmplOut) + require.Empty(t, strings.TrimSpace(string(tmplOut)), "expected empty render, got:\n%s", tmplOut) +} diff --git a/integration_test.go b/integration_test.go index 8b24b9c..565ad1a 100644 --- a/integration_test.go +++ b/integration_test.go @@ -265,6 +265,39 @@ func TestIntegration(t *testing.T) { chart: "./testdata/charts/importvalues", }) + // SAVE_SNAPSHOT=1 go test -run ^TestIntegration/empty_render_no_op$ ./ + // Tests that a chart whose templates all render to nothing (e.g. gated behind a falsy + // conditional) is treated as a no-op rather than causing an assertion error. + // See https://github.com/helmfile/chartify/issues/206 + runTest(t, integrationTestCase{ + description: "empty render no op", + release: "myapp", + chart: "./testdata/charts/emptychart", + opts: ChartifyOpts{ + // OverrideNamespace ensures ReplaceWithRendered is called even though + // no Patches/Injectors are configured, exercising the empty-render path. + OverrideNamespace: "test-ns", + }, + }) + + // SAVE_SNAPSHOT=1 go test -run ^TestIntegration/empty_render_with_patch$ ./ + // Tests that an empty render is a no-op success even when StrategicMergePatches are + // configured: the kustomize step is skipped because there are no rendered resources + // to patch. See https://github.com/helmfile/chartify/issues/206 + runTest(t, integrationTestCase{ + description: "empty render with patch", + release: "myapp", + chart: "./testdata/charts/emptychart", + opts: ChartifyOpts{ + // StrategicMergePatches forces the kustomize-build path (needsKustomizeBuild) + // which in turn requires ReplaceWithRendered. With nothing rendered, Patch must + // be skipped rather than fed an empty resource list. + StrategicMergePatches: []string{ + "./testdata/chart_patch/configmap.emptychart.strategic.yaml", + }, + }, + }) + // // Kubernets Manifests // diff --git a/replace.go b/replace.go index d83cfd2..bbdbb7f 100644 --- a/replace.go +++ b/replace.go @@ -132,72 +132,97 @@ func (r *Runner) ReplaceWithRendered(name, chartName, chartPath string, o Replac return nil, fmt.Errorf("unable to read helm output dir entries: %w", err) } - // This directory contains templates/ and charts/SUBCHART/templates + // chartOutputDir is the rendered chart directory under helmOutputDir (e.g. + // ".../helmx.1.rendered/"). It is left empty when the chart rendered + // nothing; in that case there are no rendered files to splice back into the chart. var chartOutputDir string - for _, e := range helmOutputDirEntries { - if !e.IsDir() { - return nil, fmt.Errorf("encountered unexpected dir entry at %s: it must be a dir but was not", e.Name()) + if len(helmOutputDirEntries) == 0 { + // When the chart renders no resources (e.g. every template is gated behind a + // falsy conditional), `helm template --output-dir` produces an empty directory. + // + // Remove the chart's content dirs (templates/, charts/, crds/) so that subsequent + // helm processing also sees no resources, and clean up the temp output dir. We do + // NOT return here on purpose: the Chart.yaml `dependencies` field and the lock + // files below still need to be cleaned up to avoid downstream errors like + // "found in Chart.yaml, but missing in charts/ directory". writtenFiles stays + // empty, so an empty result list is ultimately returned to the caller. + r.Logf("chart %q rendered no resources; treating empty helm output as a no-op render", chartName) + if err := os.RemoveAll(helmOutputDir); err != nil { + return nil, fmt.Errorf("cleaning up empty helm output dir %s: %w", helmOutputDir, err) } - - if chartOutputDir != "" { - return nil, fmt.Errorf("assertion failed: there should be only one dir entry under the helm output dir %s", chartOutputDir) + for _, d := range ContentDirs { + origDir := filepath.Join(chartPath, d) + if err := os.RemoveAll(origDir); err != nil { + return nil, fmt.Errorf("removing %s after empty render: %w", origDir, err) + } } + } else { + // This directory contains templates/ and charts/SUBCHART/templates + for _, e := range helmOutputDirEntries { + if !e.IsDir() { + return nil, fmt.Errorf("encountered unexpected dir entry at %s: it must be a dir but was not", e.Name()) + } - chartOutputDir = filepath.Join(helmOutputDir, e.Name()) - } + if chartOutputDir != "" { + return nil, fmt.Errorf("assertion failed: there should be only one dir entry under the helm output dir %s", chartOutputDir) + } - if !filepath.IsAbs(chartOutputDir) { - return nil, fmt.Errorf("assertion failed: unexpected dir entry %q it must be the abs path to the output directory", chartOutputDir) - } + chartOutputDir = filepath.Join(helmOutputDir, e.Name()) + } - // - Replace templates/**/*.yaml with rendered templates/**/*.yaml - // - Replace charts/SUBCHART.tgz with rendered charts/SUBCHART/templates/*.yaml - // - Replace crds/*.yaml with rendered crds/*.yaml - for _, d := range ContentDirs { - origDir := filepath.Join(chartPath, d) - if err := os.RemoveAll(origDir); err != nil { - return nil, err + if !filepath.IsAbs(chartOutputDir) { + return nil, fmt.Errorf("assertion failed: unexpected dir entry %q it must be the abs path to the output directory", chartOutputDir) } - newDir := filepath.Join(chartOutputDir, d) - if _, err := os.Stat(newDir); err != nil { - if os.IsNotExist(err) { - continue + // - Replace templates/**/*.yaml with rendered templates/**/*.yaml + // - Replace charts/SUBCHART.tgz with rendered charts/SUBCHART/templates/*.yaml + // - Replace crds/*.yaml with rendered crds/*.yaml + for _, d := range ContentDirs { + origDir := filepath.Join(chartPath, d) + if err := os.RemoveAll(origDir); err != nil { + return nil, err } - return nil, err - } - if err := os.Rename(newDir, origDir); err != nil { - return nil, err - } - usedDir := filepath.Join(chartPath, "files", d) - if err := os.RemoveAll(usedDir); err != nil && !os.IsNotExist(err) { - return nil, err + newDir := filepath.Join(chartOutputDir, d) + if _, err := os.Stat(newDir); err != nil { + if os.IsNotExist(err) { + continue + } + return nil, err + } + if err := os.Rename(newDir, origDir); err != nil { + return nil, err + } + + usedDir := filepath.Join(chartPath, "files", d) + if err := os.RemoveAll(usedDir); err != nil && !os.IsNotExist(err) { + return nil, err + } } - } - lines := strings.Split(stdout, "\n") - for _, line := range lines { - if strings.HasPrefix(line, "wrote ") { - file := strings.Split(line, "wrote ")[1] + lines := strings.Split(stdout, "\n") + for _, line := range lines { + if strings.HasPrefix(line, "wrote ") { + file := strings.Split(line, "wrote ")[1] - for _, d := range ContentDirs { - origDir := filepath.Join(chartPath, d) - newDir := filepath.Join(chartOutputDir, d) - file = strings.ReplaceAll(strings.ReplaceAll(file, "/", string(filepath.Separator)), newDir, origDir) - } + for _, d := range ContentDirs { + origDir := filepath.Join(chartPath, d) + newDir := filepath.Join(chartOutputDir, d) + file = strings.ReplaceAll(strings.ReplaceAll(file, "/", string(filepath.Separator)), newDir, origDir) + } - writtenFiles[file] = true + writtenFiles[file] = true + } } - } - if len(writtenFiles) == 0 { - return nil, fmt.Errorf("invalid state: no files rendered") - } + if len(writtenFiles) == 0 { + return nil, fmt.Errorf("invalid state: no files rendered") + } - if err := os.RemoveAll(helmOutputDir); err != nil { - return nil, fmt.Errorf("cleaning up unnecessary files after replace: %v", err) + if err := os.RemoveAll(helmOutputDir); err != nil { + return nil, fmt.Errorf("cleaning up unnecessary files after replace: %w", err) + } } results := make([]string, 0, len(writtenFiles)) diff --git a/testdata/chart_patch/configmap.emptychart.strategic.yaml b/testdata/chart_patch/configmap.emptychart.strategic.yaml new file mode 100644 index 0000000..83091fb --- /dev/null +++ b/testdata/chart_patch/configmap.emptychart.strategic.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: myapp-cm +data: + patched: "true" diff --git a/testdata/charts/emptychart/Chart.yaml b/testdata/charts/emptychart/Chart.yaml new file mode 100644 index 0000000..2613056 --- /dev/null +++ b/testdata/charts/emptychart/Chart.yaml @@ -0,0 +1,6 @@ +apiVersion: v2 +name: emptychart +description: A Helm chart whose templates are all conditionally disabled, used to test empty render handling +type: application +version: 0.1.0 +appVersion: "1.0.0" diff --git a/testdata/charts/emptychart/templates/configmap.yaml b/testdata/charts/emptychart/templates/configmap.yaml new file mode 100644 index 0000000..a5668cd --- /dev/null +++ b/testdata/charts/emptychart/templates/configmap.yaml @@ -0,0 +1,8 @@ +{{- if .Values.enabled }} +apiVersion: v1 +kind: ConfigMap +metadata: + name: {{ .Release.Name }}-cm +data: + hello: world +{{- end }} diff --git a/testdata/charts/emptychart/values.yaml b/testdata/charts/emptychart/values.yaml new file mode 100644 index 0000000..5cb6585 --- /dev/null +++ b/testdata/charts/emptychart/values.yaml @@ -0,0 +1,3 @@ +# When enabled is false (the default), this chart renders no resources, +# which exercises the empty-render code path in chartify. +enabled: false diff --git a/testdata/integration/testcases/empty_render_no_op/want b/testdata/integration/testcases/empty_render_no_op/want new file mode 100644 index 0000000..8b13789 --- /dev/null +++ b/testdata/integration/testcases/empty_render_no_op/want @@ -0,0 +1 @@ + diff --git a/testdata/integration/testcases/empty_render_with_patch/want b/testdata/integration/testcases/empty_render_with_patch/want new file mode 100644 index 0000000..8b13789 --- /dev/null +++ b/testdata/integration/testcases/empty_render_with_patch/want @@ -0,0 +1 @@ +