From f0e418026a07e36ba2a6d59366d062ffdc5a09c4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Aug 2026 00:06:08 +0000 Subject: [PATCH 1/4] Initial plan From d38b67fed1fb45e00de80b6a74e15f151d446f28 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 1 Aug 2026 00:15:55 +0000 Subject: [PATCH 2/4] Fix empty render crash: treat no-output helm template as no-op When helm template --output-dir produces an empty directory (e.g. all templates are gated behind a falsy conditional), ReplaceWithRendered now removes the chart's content dirs and returns successfully with an empty file list instead of crashing with an assertion error. Adds test chart testdata/charts/emptychart and integration test 'empty render no op' to cover this scenario. Closes #206 --- integration_test.go | 15 +++++++++++++++ replace.go | 17 +++++++++++++++++ testdata/charts/emptychart/Chart.yaml | 6 ++++++ .../charts/emptychart/templates/configmap.yaml | 8 ++++++++ testdata/charts/emptychart/values.yaml | 3 +++ .../testcases/empty_render_no_op/want | 1 + 6 files changed, 50 insertions(+) create mode 100644 testdata/charts/emptychart/Chart.yaml create mode 100644 testdata/charts/emptychart/templates/configmap.yaml create mode 100644 testdata/charts/emptychart/values.yaml create mode 100644 testdata/integration/testcases/empty_render_no_op/want diff --git a/integration_test.go b/integration_test.go index 8b24b9c..49d881e 100644 --- a/integration_test.go +++ b/integration_test.go @@ -265,6 +265,21 @@ func TestIntegration(t *testing.T) { chart: "./testdata/charts/importvalues", }) + // SAVE_SNAPSHOT=1 go1.25 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", + }, + }) + // // Kubernets Manifests // diff --git a/replace.go b/replace.go index d83cfd2..04a3304 100644 --- a/replace.go +++ b/replace.go @@ -132,6 +132,23 @@ func (r *Runner) ReplaceWithRendered(name, chartName, chartPath string, o Replac return nil, fmt.Errorf("unable to read helm output dir entries: %w", err) } + // When the chart renders no resources, helm template --output-dir produces an empty + // directory. Treat this as a no-op: remove original content dirs so that subsequent + // helm processing also sees no resources, clean up the temp output dir, and return + // an empty file list with no error. + if len(helmOutputDirEntries) == 0 { + if err := os.RemoveAll(helmOutputDir); err != nil { + return nil, fmt.Errorf("cleaning up empty helm output dir: %v", err) + } + for _, d := range ContentDirs { + origDir := filepath.Join(chartPath, d) + if err := os.RemoveAll(origDir); err != nil { + return nil, err + } + } + return nil, nil + } + // This directory contains templates/ and charts/SUBCHART/templates var chartOutputDir string 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 @@ + From f2507b1aa6bf66a502aa12010d1a20af9b2e9e90 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Mon, 3 Aug 2026 07:04:07 +0800 Subject: [PATCH 3/4] Complete the empty-render fix: clean deps, skip kustomize, harden tests Addresses the gaps identified in review of the empty-render fix (#206): 1. replace.go: the empty-render branch no longer early-returns. It removes the content dirs and temp output, then falls through so the Chart.yaml `dependencies` field and Chart.lock/requirements.lock cleanup still run. Without this, an empty-rendering chart that declares dependencies would leave Chart.yaml referencing subcharts whose charts/ dir was removed, causing a downstream "found in Chart.yaml, but missing in charts/ directory" error. Also fixes %v -> %w wrapping and adds path context. 2. chartify.go: skip the kustomize Patch step when no resources were rendered (`len(generatedManifestFiles) == 0`). Previously an empty render combined with JsonPatches/StrategicMergePatches/Transformers fed kustomize an empty resource list plus patches, failing with "no resource matches strategic merge patch ...". An empty render is now a no-op success even when patches are configured (there is simply nothing to patch). Tests: - TestEmptyRenderCleansChartDependencies: verifies a chart that declares a dependency and renders nothing still produces a chartifiable output whose Chart.yaml has no dependencies and templates empty. Proven to fail without fix #1. - integration "empty render with patch": exercises the StrategicMergePatches path on the empty chart; proven to fail without fix #2. - testdata/chart_patch/configmap.emptychart.strategic.yaml + empty snapshot. Signed-off-by: yxxhero --- chartify.go | 6 +- chartify_test.go | 78 +++++++++++ integration_test.go | 18 +++ replace.go | 124 ++++++++++-------- .../configmap.emptychart.strategic.yaml | 6 + .../testcases/empty_render_with_patch/want | 1 + 6 files changed, 174 insertions(+), 59 deletions(-) create mode 100644 testdata/chart_patch/configmap.emptychart.strategic.yaml create mode 100644 testdata/integration/testcases/empty_render_with_patch/want 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 49d881e..6a47d9c 100644 --- a/integration_test.go +++ b/integration_test.go @@ -280,6 +280,24 @@ func TestIntegration(t *testing.T) { }, }) + // SAVE_SNAPSHOT=1 go1.25 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 04a3304..bbdbb7f 100644 --- a/replace.go +++ b/replace.go @@ -132,89 +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) } - // When the chart renders no resources, helm template --output-dir produces an empty - // directory. Treat this as a no-op: remove original content dirs so that subsequent - // helm processing also sees no resources, clean up the temp output dir, and return - // an empty file list with no error. + // 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 + 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: %v", err) + return nil, fmt.Errorf("cleaning up empty helm output dir %s: %w", helmOutputDir, err) } for _, d := range ContentDirs { origDir := filepath.Join(chartPath, d) if err := os.RemoveAll(origDir); err != nil { - return nil, err + return nil, fmt.Errorf("removing %s after empty render: %w", origDir, err) } } - return nil, nil - } + } 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()) + } - // This directory contains templates/ and charts/SUBCHART/templates - var chartOutputDir string + if chartOutputDir != "" { + return nil, fmt.Errorf("assertion failed: there should be only one dir entry under the helm output dir %s", chartOutputDir) + } - 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()) - } - - 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) - } + // - 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 + } - // - 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 - } + 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 + } - newDir := filepath.Join(chartOutputDir, d) - if _, err := os.Stat(newDir); err != nil { - if os.IsNotExist(err) { - continue + usedDir := filepath.Join(chartPath, "files", d) + if err := os.RemoveAll(usedDir); err != nil && !os.IsNotExist(err) { + 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 - } - } + 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/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 @@ + From 9656218b0fe7916d17955bbd33f4be08a3d08568 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Mon, 3 Aug 2026 07:17:22 +0800 Subject: [PATCH 4/4] Address review: use unversioned `go test` in example commands The SAVE_SNAPSHOT example comments used `go1.25 test`, but this repo's go.mod requires Go 1.26.0+, so `go1.25 test` would fail. Switch to the unversioned `go test` (reviewer's preferred form) on the two empty-render test cases flagged in review. Signed-off-by: yxxhero --- integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration_test.go b/integration_test.go index 6a47d9c..565ad1a 100644 --- a/integration_test.go +++ b/integration_test.go @@ -265,7 +265,7 @@ func TestIntegration(t *testing.T) { chart: "./testdata/charts/importvalues", }) - // SAVE_SNAPSHOT=1 go1.25 test -run ^TestIntegration/empty_render_no_op$ ./ + // 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 @@ -280,7 +280,7 @@ func TestIntegration(t *testing.T) { }, }) - // SAVE_SNAPSHOT=1 go1.25 test -run ^TestIntegration/empty_render_with_patch$ ./ + // 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