public_git/helmfile
1
0
Fork 0
mirror of https://github.com/helmfile/helmfile.git synced 2026-10-21 12:15:12 +02:00
Code Issues Projects Releases Wiki Activity

refactor: drop empty-render workaround now that chartify handles it natively (#2747)

Browse Source 
Bump github.com/helmfile/chartify to v0.28.2, which fixes the empty-render
crash upstream (helmfile/chartify#206, fixed in helmfile/chartify#207):
when a chart renders zero resources, chartify now treats it as a no-op
success itself - removing the chart's content dirs, cleaning Chart.yaml
dependencies and the lock file, and skipping the kustomize step - instead
of failing with:

  assertion failed: unexpected dir entry "" it must be the abs path to the output directory

That makes the helmfile-side string-matching workaround from #2724 dead
code: the error it matched can no longer be produced. Remove
isChartifyEmptyRenderOutputError and the special-cased no-op branch in
processChartification, so the empty-render case simply flows through the
regular chartify path.

Also bump github.com/helmfile/vals to v0.46.0 and refresh transitive
dependencies.

The regression test for #1757 is updated to assert the new direct
behavior: processChartification succeeds, returns a chartified chart
whose Chart.yaml no longer declares dependencies (so a subsequent
"helm dep build"/"helm template" cannot fail with "found in Chart.yaml,
but missing in charts/ directory"), renders empty output, and is cleaned
up by CleanupChartifyTempDirs.

Fixes #1757 (follow-up to #2724)

Signed-off-by: yxxhero <aiopsclub@163.com>
This commit is contained in:
yxxhero
2026-08-16 10:47:19 +08:00
committed by GitHub
parent e178870ce3
commit 6034a54e0d
4 changed files with 161 additions and 172 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/state/issue_1757_test.go
+55 -89
View File
@@ -1,11 +1,11 @@
package state
import (
"errors"
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"testing"
"github.com/helmfile/chartify"
@@ -16,91 +16,40 @@ import (
"github.com/helmfile/helmfile/pkg/filesystem"
)
// TestIsChartifyEmptyRenderOutputError verifies that isChartifyEmptyRenderOutputError
// only matches chartify's specific "empty rendered output dir" assertion failure, not
// other, unrelated chartify errors. This is a regression test for issue #1757: a chart
// that renders zero resources (e.g. everything gated behind a falsy `if`) combined with
// TestProcessChartification_EmptyRender is an end-to-end regression test for
// issue #1757: a chart that renders zero resources (e.g. everything gated
// behind a falsy `{{- if .Values.enabled }}`) combined with
// transformers/jsonPatches/strategicMergePatches used to crash helmfile with
//
// assertion failed: unexpected dir entry "" it must be the abs path to the output directory
//
// because chartify expects exactly one rendered output directory and found none.
func TestIsChartifyEmptyRenderOutputError(t *testing.T) {
tests := []struct {
name string
err error
expected bool
}{
{
name: "nil error",
err: nil,
expected: false,
},
{
name: "chartify empty render assertion error",
err: errors.New(`assertion failed: unexpected dir entry "" it must be the abs path to the output directory`),
expected: true,
},
{
// Same assertion, but chartOutputDir is a non-empty (merely
// relative) path rather than "" - a different, hypothetical
// chartify bug that happens to trip the same final assertion.
// This must NOT be treated as the empty-render no-op case:
// per review feedback (https://github.com/helmfile/helmfile/pull/2724),
// matching only on the trailing "...it must be the abs path to
// the output directory" phrase (without requiring the `""`
// empty-string dir entry) would have incorrectly matched this
// too, silently masking a genuinely different failure.
name: "same assertion with a non-empty dir entry is not the empty-render case",
err: errors.New(`assertion failed: unexpected dir entry "relative/path" it must be the abs path to the output directory`),
expected: false,
},
{
name: "unrelated chartify error",
err: errors.New("exec: \"kustomize\": executable file not found in %PATH%"),
expected: false,
},
{
name: "unrelated multiple-dir-entries assertion",
err: errors.New(`assertion failed: there should be only one dir entry under the helm output dir /tmp/foo`),
expected: false,
},
{
name: "helm template failure",
err: errors.New("Error: template: mychart/templates/broken.yaml:3:5: executing..."),
expected: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, isChartifyEmptyRenderOutputError(tt.err))
})
}
}
// TestProcessChartification_EmptyRenderReturnsSurvivingPath is an end-to-end
// regression test for a bug found in review of the #1757 fix: the empty-render
// no-op path must return the chart's original path, not the temp copy that
// rewriteChartDependencies creates when the chart has relative file:// deps -
// that temp dir is removed by a deferred cleanup as soon as processChartification
// returns, so returning it would hand the caller a path to a directory that no
// longer exists on disk.
// because chartify (<= v0.28.1) expects exactly one rendered output directory
// and aborts when it finds none. The initial fix in helmfile (#2724) worked
// around this by string-matching that assertion error and skipping
// chartification entirely. Chartify v0.28.2 (see
// https://github.com/helmfile/chartify/issues/206 and its fix in
// https://github.com/helmfile/chartify/pull/207) now treats an empty render as
// a no-op success internally - it removes the chart's content dirs, cleans up
// Chart.yaml `dependencies` and the lock file, and skips the kustomize step -
// so the helmfile-side workaround was removed. This test guards the resulting
// direct behavior: processChartification must succeed on an empty render and
// hand back a chartified chart that is still usable by subsequent helm
// commands (`helm dep build`-safe Chart.yaml, empty `helm template` output).
//
// This exercises the real processChartification -> chartify.Chartify wiring
// (unlike TestIsChartifyEmptyRenderOutputError above, which only tests the pure
// string-matching helper), so it needs real helm and kustomize binaries on PATH
// and is skipped if either is missing. Verified to pass on Linux; skipped on
// Windows because the file:// dependency URL this test needs (to make
// rewriteChartDependencies actually produce a temp copy) hits an unrelated,
// pre-existing Windows path-handling issue in chartify/helm's dependency
// resolution (a Windows drive letter embedded in a file:// URL gets
// mis-joined onto another path), independent of the fix under test here.
func TestProcessChartification_EmptyRenderReturnsSurvivingPath(t *testing.T) {
// It exercises the real processChartification -> chartify.Chartify wiring, so
// it needs real helm and kustomize binaries on PATH and is skipped if either
// is missing. Verified to pass on Linux; skipped on Windows because the
// file:// dependency URL this test needs (to make rewriteChartDependencies
// actually produce a temp copy) hits an unrelated, pre-existing Windows
// path-handling issue in chartify/helm's dependency resolution (a Windows
// drive letter embedded in a file:// URL gets mis-joined onto another path),
// independent of the code path under test here.
func TestProcessChartification_EmptyRender(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("skipping on Windows: unrelated file:// dependency URL / drive letter handling issue in chartify's helm dependency resolution, not the code path under test; passes on Linux (CI)")
}
if _, err := exec.LookPath("helm"); err != nil {
helmBin := "helm"
if _, err := exec.LookPath(helmBin); err != nil {
t.Skip("helm not found on PATH, skipping")
}
if _, err := exec.LookPath("kustomize"); err != nil {
@@ -134,8 +83,8 @@ dependencies:
`), 0644))
require.NoError(t, os.WriteFile(filepath.Join(chartDir, "values.yaml"), []byte("enabled: false\n"), 0644))
// Every template is gated behind a falsy condition, so helm renders zero
// resources - the exact condition that triggers chartify's empty-output
// assertion.
// resources - the exact condition that used to trigger chartify's
// empty-output assertion.
require.NoError(t, os.WriteFile(filepath.Join(chartDir, "templates", "deployment.yaml"), []byte(`
{{- if .Values.enabled }}
apiVersion: apps/v1
@@ -165,7 +114,7 @@ fieldSpecs:
logger: zap.NewNop().Sugar(),
fs: filesystem.DefaultFileSystem(),
ReleaseSetSpec: ReleaseSetSpec{
DefaultHelmBinary: "helm",
DefaultHelmBinary: helmBin,
DefaultKustomizeBinary: "kustomize",
},
}
@@ -182,19 +131,36 @@ fieldSpecs:
chartification, release, chartDir, ChartPrepareOptions{}, false, "template",
)
// The core #1757 regression: no "assertion failed: unexpected dir entry ..."
// error, chartify (>= v0.28.2) treats the empty render as a no-op.
require.NoError(t, err)
assert.True(t, buildDeps, "buildDeps should be true (!skipDeps) for the no-op path")
assert.True(t, buildDeps, "buildDeps should be true (!skipDeps)")
// The returned path must still exist: it must be the original chart
// directory, not the deps-rewritten temp copy that gets deleted by
// rewriteChartDependencies' deferred cleanup on return.
// The returned path is chartify's output dir; it must exist and contain a Chart.yaml.
info, statErr := os.Stat(resultPath)
require.NoError(t, statErr, "returned chart path %q must still exist after processChartification returns", resultPath)
assert.True(t, info.IsDir())
// It should specifically be the chart's own directory (or an
// equally-valid, not-yet-cleaned-up path to the same chart), not some
// other temp directory. Chart.yaml must be readable from it.
_, err = os.Stat(filepath.Join(resultPath, "Chart.yaml"))
assert.NoError(t, err, "Chart.yaml should be reachable from the returned path %q", resultPath)
// On the empty-render path chartify removes the Chart.yaml `dependencies`
// field (together with charts/ and Chart.lock), so that a subsequent
// `helm dependency build`/`helm template` on the chartified output does not
// fail with "found in Chart.yaml, but missing in charts/ directory".
chartYaml, readErr := os.ReadFile(filepath.Join(resultPath, "Chart.yaml"))
require.NoError(t, readErr)
assert.NotContainsf(t, string(chartYaml), "dependencies:",
"Chart.yaml dependencies field should have been removed after an empty render; got:\n%s", chartYaml)
// The chartified output must render to (empty) nothing without any further
// preparation, proving it is directly usable by subsequent helm commands.
tmplOut, tmplErr := exec.Command(helmBin, "template", release.Name, resultPath).CombinedOutput()
require.NoErrorf(t, tmplErr, "helm template on chartified output failed: %s", tmplOut)
assert.Empty(t, strings.TrimSpace(string(tmplOut)), "expected empty render, got:\n%s", tmplOut)
// processChartification tracked the output dir for deferred cleanup; after
// cleanup the dir (and its parent temp dir) must be gone.
st.CleanupChartifyTempDirs()
_, statErr = os.Stat(resultPath)
assert.True(t, os.IsNotExist(statErr), "chartify temp dir %q should have been removed by CleanupChartifyTempDirs", resultPath)
}
pkg/state/state.go
-48
View File
@@ -1873,11 +1873,6 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (string, func(),
// If exists, it will also patch resources by json patches, strategic-merge patches, and injectors.
// processChartification handles the chartification process
func (st *HelmState) processChartification(chartification *Chartify, release *ReleaseSpec, chartPath string, opts ChartPrepareOptions, skipDeps bool, helmfileCommand string) (string, bool, error) {
// Preserved so the empty-render no-op path below can return this instead of the
// deps-rewritten temp copy assigned to chartPath further down, which the deferred
// cleanupTempChart() removes as soon as this function returns.
originalChartPath := chartPath
// Rewrite relative file:// dependencies in Chart.yaml to absolute paths before chartify processes them
// This prevents errors like "Error: directory /tmp/chartify.../argocd-application not found"
// when Chart.yaml contains dependencies like "file://../argocd-application"
@@ -1949,21 +1944,6 @@ func (st *HelmState) processChartification(chartification *Chartify, release *Re
)
out, err := c.Chartify(release.Name, chartPath, chartify.WithChartifyOpts(chartifyOpts))
if err != nil && isChartifyEmptyRenderOutputError(err) {
// The chart rendered zero resources (e.g. everything is gated behind a
// `{{- if .Values.enabled }}` that evaluated to false), so chartify has
// nothing to replace templates/charts/crds with and fails its internal
// "there must be exactly one rendered output dir" assertion. Treat this
// as a no-op: use the chart as-is, since there's nothing to chartify.
// See https://github.com/helmfile/helmfile/issues/1757 and the upstream
// tracking issue https://github.com/helmfile/chartify/issues/206.
st.logger.Debugf("release %q: chart rendered no resources, skipping chartification: %v", release.Name, err)
// Return originalChartPath, NOT chartPath: chartPath may have been reassigned
// above to the deps-rewritten temp copy, which the deferred cleanupTempChart()
// removes as soon as this function returns. Since chartify never actually ran,
// relative file:// deps resolve fine from the original location anyway.
return originalChartPath, !skipDeps, nil
}
if err != nil {
return "", false, err
}
@@ -1979,34 +1959,6 @@ func (st *HelmState) processChartification(chartification *Chartify, release *Re
return chartPath, buildDeps, nil
}
// chartifyEmptyRenderOutputErrSubstring is the distinctive part of the error that
// github.com/helmfile/chartify (as of v0.28.0, see replace.go) returns when `helm template`
// renders zero resources for a chart: it expects exactly one directory entry under its
// `--output-dir`, and an empty render leaves that directory empty, so the "must be the abs
// path to the output directory" assertion fails on the resulting empty string. Chartify does
// not expose a typed/sentinel error for this case, so we match on the error text.
//
// The matched string includes the `unexpected dir entry ""` prefix (not just the trailing
// "...it must be the abs path to the output directory" phrase) so this can only match when
// chartify's chartOutputDir was genuinely empty - i.e. the exact empty-render case - and not
// some other, hypothetical failure of the same assertion against a non-empty (but still
// relative) path, which would be a different bug that should still be surfaced as an error.
//
// If this substring ever stops matching a real chartify error, chartify's wording has
// changed and this check needs to be revisited.
//
// Tracked upstream at https://github.com/helmfile/chartify/issues/206 - once chartify
// exposes a sentinel error (or treats an empty render as a no-op itself) and this repo
// bumps to that version, this text match can be retired.
const chartifyEmptyRenderOutputErrSubstring = `unexpected dir entry "" it must be the abs path to the output directory`
// isChartifyEmptyRenderOutputError reports whether err is chartify's assertion failure caused
// by a chart rendering zero resources, as opposed to some other, unrelated chartify failure
// that should still be surfaced to the user.
func isChartifyEmptyRenderOutputError(err error) bool {
return err != nil && strings.Contains(err.Error(), chartifyEmptyRenderOutputErrSubstring)
}
func (st *HelmState) appendSkipSchemaValidationFlagToChartifyTemplateArgs(templateArgs string, release *ReleaseSpec, skipSchemaValidation bool) string {
if !st.shouldSkipSchemaValidation(release, skipSchemaValidation) || hasTemplateArg(templateArgs, "--skip-schema-validation") {
return templateArgs