From 47c7b75ba30e7076debff948a7b65fb8dd216790 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Tue, 10 Mar 2026 07:38:53 +0800 Subject: [PATCH] fix: --include-needs should only include direct dependencies (#1003) --include-needs was incorrectly including transitive dependencies when it should only include direct dependencies per the documentation. The issue was that ChartPrepareOptions.IncludeTransitiveNeeds was being set to c.IncludeNeeds() instead of c.IncludeTransitiveNeeds() in the chart preparation phase for diff, template, lint, sync, and apply commands. This fix ensures: - --include-needs includes only direct dependencies - --include-transitive-needs includes both direct and transitive dependencies Fixes #1003 Signed-off-by: yxxhero Signed-off-by: yxxhero (cherry picked from commit 7a2cbeebb861acc4caab36c405cd891959c65691) Signed-off-by: yxxhero --- pkg/app/app.go | 10 +- pkg/app/app_template_test.go | 3 +- test/integration/run.sh | 1 + .../test-cases/include-needs-transitive.sh | 100 ++++++++++++++++++ .../include-needs-transitive/README.md | 35 ++++++ .../input/helmfile.yaml | 23 ++++ 6 files changed, 165 insertions(+), 7 deletions(-) create mode 100755 test/integration/test-cases/include-needs-transitive.sh create mode 100644 test/integration/test-cases/include-needs-transitive/README.md create mode 100644 test/integration/test-cases/include-needs-transitive/input/helmfile.yaml diff --git a/pkg/app/app.go b/pkg/app/app.go index cdc31745..95a0bf3d 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -169,7 +169,7 @@ func (a *App) Diff(c DiffConfigProvider) error { IncludeCRDs: &includeCRDs, Validate: c.Validate(), Concurrency: c.Concurrency(), - IncludeTransitiveNeeds: c.IncludeNeeds(), + IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), }, func() { msg, matched, affected, errs = a.diff(run, c) }) @@ -240,7 +240,7 @@ func (a *App) Template(c TemplateConfigProvider) error { SkipCleanup: c.SkipCleanup(), Validate: c.Validate(), Concurrency: c.Concurrency(), - IncludeTransitiveNeeds: c.IncludeNeeds(), + IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), Set: c.Set(), Values: c.Values(), KubeVersion: c.KubeVersion(), @@ -317,7 +317,7 @@ func (a *App) Lint(c LintConfigProvider) error { SkipDeps: c.SkipDeps(), SkipCleanup: c.SkipCleanup(), Concurrency: c.Concurrency(), - IncludeTransitiveNeeds: c.IncludeNeeds(), + IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), }, func() { ok, lintErrs, errs = a.lint(run, c) }) @@ -417,7 +417,7 @@ func (a *App) Sync(c SyncConfigProvider) error { WaitRetries: c.WaitRetries(), WaitForJobs: c.WaitForJobs(), IncludeCRDs: &includeCRDs, - IncludeTransitiveNeeds: c.IncludeNeeds(), + IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), Validate: c.Validate(), Concurrency: c.Concurrency(), }, func() { @@ -455,7 +455,7 @@ func (a *App) Apply(c ApplyConfigProvider) error { SkipCleanup: c.SkipCleanup(), Validate: c.Validate(), Concurrency: c.Concurrency(), - IncludeTransitiveNeeds: c.IncludeNeeds(), + IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), }, func() { matched, updated, es := a.apply(run, c) diff --git a/pkg/app/app_template_test.go b/pkg/app/app_template_test.go index f2e5f50c..3266f509 100644 --- a/pkg/app/app_template_test.go +++ b/pkg/app/app_template_test.go @@ -205,9 +205,8 @@ releases: error: ``, selectors: []string{"app=test"}, // Issue #2309: --kube-context is now added to template flags + // Issue #1003: --include-needs should only include direct dependencies, not transitive templated: []exectest.Release{ - // TODO: Turned out we can't differentiate needs vs transitive needs in this case :thinking: - {Name: "logging", Flags: []string{"--kube-context", "default", "--namespace", "kube-system"}}, {Name: "kubernetes-external-secrets", Flags: []string{"--kube-context", "default", "--namespace", "kube-system"}}, {Name: "external-secrets", Flags: []string{"--kube-context", "default", "--namespace", "default"}}, {Name: "my-release", Flags: []string{"--kube-context", "default", "--namespace", "default"}}, diff --git a/test/integration/run.sh b/test/integration/run.sh index 80099707..946e595e 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -138,6 +138,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes . ${dir}/test-cases/issue-2431.sh . ${dir}/test-cases/kubedog-tracking.sh +. ${dir}/test-cases/include-needs-transitive.sh # ALL DONE ----------------------------------------------------------------------------------------------------------- all_tests_passed diff --git a/test/integration/test-cases/include-needs-transitive.sh b/test/integration/test-cases/include-needs-transitive.sh new file mode 100755 index 00000000..74e33f6a --- /dev/null +++ b/test/integration/test-cases/include-needs-transitive.sh @@ -0,0 +1,100 @@ +#!/usr/bin/env bash + +# Test case for issue #1003: --include-needs should only include direct dependencies +# while --include-transitive-needs should include both direct and transitive dependencies + +test_case_dir="${cases_dir}/include-needs-transitive" +input_dir="${test_case_dir}/input" +output_dir="${test_case_dir}/output" + +tmp_dir=$(mktemp -d) +trap "rm -rf ${tmp_dir}" EXIT + +test_start "include-needs vs include-transitive-needs" + +info "https://github.com/helmfile/helmfile/issues/1003" + +# Test 1: --include-needs should only include direct dependencies +info "Test 1: template with --include-needs should only include direct dependencies" +template_output="${tmp_dir}/include-needs.log" +${helmfile} -f ${input_dir}/helmfile.yaml -l name=release3 template --include-needs > ${template_output} 2>&1 +code=$? +if [ ${code} -ne 0 ]; then + cat ${template_output} + fail "helmfile template with --include-needs should not fail" +fi + +# Should include release2 (direct dependency) and release3 (selected) +# Should NOT include release1 (transitive dependency) +if grep -q "release1" ${template_output}; then + fail "--include-needs should NOT include transitive dependency release1" +fi + +if ! grep -q "release2" ${template_output}; then + fail "--include-needs should include direct dependency release2" +fi + +if ! grep -q "release3" ${template_output}; then + fail "--include-needs should include selected release release3" +fi + +info "PASS: --include-needs only includes direct dependencies" + +# Test 2: --include-transitive-needs should include all dependencies +info "Test 2: template with --include-transitive-needs should include all dependencies" +template_output="${tmp_dir}/include-transitive-needs.log" +${helmfile} -f ${input_dir}/helmfile.yaml -l name=release3 template --include-transitive-needs > ${template_output} 2>&1 +code=$? +if [ ${code} -ne 0 ]; then + cat ${template_output} + fail "helmfile template with --include-transitive-needs should not fail" +fi + +# Should include all: release1, release2, and release3 +if ! grep -q "release1" ${template_output}; then + fail "--include-transitive-needs should include transitive dependency release1" +fi + +if ! grep -q "release2" ${template_output}; then + fail "--include-transitive-needs should include direct dependency release2" +fi + +if ! grep -q "release3" ${template_output}; then + fail "--include-transitive-needs should include selected release release3" +fi + +info "PASS: --include-transitive-needs includes all dependencies" + +# Test 3: Verify same behavior for lint command +info "Test 3: lint with --include-needs should only include direct dependencies" +lint_output="${tmp_dir}/lint-include-needs.log" +${helmfile} -f ${input_dir}/helmfile.yaml -l name=release3 lint --include-needs > ${lint_output} 2>&1 +code=$? +if [ ${code} -ne 0 ]; then + cat ${lint_output} + fail "helmfile lint with --include-needs should not fail" +fi + +if grep -q "release1" ${lint_output}; then + fail "lint with --include-needs should NOT include transitive dependency release1" +fi + +info "PASS: lint with --include-needs only includes direct dependencies" + +# Test 4: Verify same behavior for diff command +info "Test 4: diff with --include-needs should only include direct dependencies" +diff_output="${tmp_dir}/diff-include-needs.log" +${helmfile} -f ${input_dir}/helmfile.yaml -l name=release3 diff --include-needs > ${diff_output} 2>&1 +code=$? +if [ ${code} -ne 0 ]; then + cat ${diff_output} + fail "helmfile diff with --include-needs should not fail" +fi + +if grep -q "release1" ${diff_output}; then + fail "diff with --include-needs should NOT include transitive dependency release1" +fi + +info "PASS: diff with --include-needs only includes direct dependencies" + +test_pass "include-needs vs include-transitive-needs" diff --git a/test/integration/test-cases/include-needs-transitive/README.md b/test/integration/test-cases/include-needs-transitive/README.md new file mode 100644 index 00000000..013ee54b --- /dev/null +++ b/test/integration/test-cases/include-needs-transitive/README.md @@ -0,0 +1,35 @@ +# Test Case: include-needs vs include-transitive-needs + +This test case validates the fix for issue #1003. + +## Issue +`--include-needs` flag was incorrectly including transitive dependencies when it should only include direct dependencies. + +## Expected Behavior + +### --include-needs +When selecting `release3` with `--include-needs`: +- **Should include**: `release2` (direct dependency) and `release3` (selected release) +- **Should NOT include**: `release1` (transitive dependency of release3 through release2) + +### --include-transitive-needs +When selecting `release3` with `--include-transitive-needs`: +- **Should include**: `release1`, `release2`, and `release3` (all dependencies in the chain) + +## Dependency Chain +``` +release1 (no dependencies) + ↑ +release2 (needs release1) + ↑ +release3 (needs release2) +``` + +## Tests Performed +1. `helmfile template --include-needs` - verifies only direct dependencies are included +2. `helmfile template --include-transitive-needs` - verifies all dependencies are included +3. `helmfile lint --include-needs` - verifies consistency across commands +4. `helmfile diff --include-needs` - verifies consistency across commands + +## References +- Issue: https://github.com/helmfile/helmfile/issues/1003 diff --git a/test/integration/test-cases/include-needs-transitive/input/helmfile.yaml b/test/integration/test-cases/include-needs-transitive/input/helmfile.yaml new file mode 100644 index 00000000..bf4b6a11 --- /dev/null +++ b/test/integration/test-cases/include-needs-transitive/input/helmfile.yaml @@ -0,0 +1,23 @@ +repositories: + - name: incubator + url: https://charts.helm.sh/incubator + +releases: + # release1 has no dependencies - this is a transitive dependency of release3 + - name: release1 + chart: incubator/raw + namespace: default + + # release2 depends on release1 - this is a direct dependency of release3 + - name: release2 + chart: incubator/raw + namespace: default + needs: + - default/release1 + + # release3 depends on release2 (and transitively on release1) + - name: release3 + chart: incubator/raw + namespace: default + needs: + - default/release2