From 5ca2cbb68dd605c208de3ae18a018d52991f6855 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 19 Apr 2026 12:04:23 +0800 Subject: [PATCH] fix: add debug logging, unit tests, docs, and fix integration test for subhelmfile selector skip - Add debug log when skipping subhelmfile due to selector conflict - Add TestSubhelmfileSelectorsConflict with 11 cases for direct unit coverage - Document the selector-based subhelmfile skip optimization in docs/index.md - Fix integration test: use 'app' label key instead of reserved 'name' key (GetReleasesWithLabels overwrites labels["name"] with the release name) Signed-off-by: yxxhero --- docs/index.md | 1 + pkg/app/app.go | 1 + pkg/app/app_test.go | 83 +++++++++++++++++++ test/integration/test-cases/issue-2544.sh | 32 +++---- .../test-cases/issue-2544/input/helmfile.yaml | 6 +- .../issue-2544/input/subhelmfile-a.yaml | 2 +- .../issue-2544/input/subhelmfile-b.yaml | 2 +- .../issue-2544/input/subhelmfile-c.yaml | 2 +- 8 files changed, 107 insertions(+), 22 deletions(-) diff --git a/docs/index.md b/docs/index.md index 218d9d10..36eff1a7 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1523,6 +1523,7 @@ helmfiles: ``` * When a selector is specified, only this selector applies and the parents or CLI selectors are ignored. +* When CLI selectors are provided (e.g. `helmfile -l name=b sync`) and a subhelmfile has explicit selectors that are provably incompatible (same key, different value), that subhelmfile is **skipped entirely** without loading or rendering it. For example, with `-l name=b`, a subhelmfile with `selectors: [name=a]` will be skipped since no release could match both. This optimization does not apply when `selectorsInherited: true` is set or when no CLI selectors are provided. Use `--debug` to see log messages about skipped subhelmfiles. * When not selector is specified there are 2 modes for the selector inheritance because we would like to change the current inheritance behavior (see [issue #344](https://github.com/roboll/helmfile/issues/344) ). * Legacy mode, sub-helmfiles without selectors inherit selectors from their parent helmfile. The initial helmfiles inherit from the command line selectors. * explicit mode, sub-helmfile without selectors do not inherit from their parent or the CLI selector. If you want them to inherit from their parent selector then use `selectorsInherited: true`. To enable this explicit mode you need to set the following environment variable `HELMFILE_EXPERIMENTAL=explicit-selector-inheritance` (see [experimental](#experimental-features)). diff --git a/pkg/app/app.go b/pkg/app/app.go index 37b5b514..f4eede7d 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1008,6 +1008,7 @@ func (a *App) processNestedHelmfiles(st *state.HelmState, absd, file string, def anyMatched := false for i, m := range st.Helmfiles { if subhelmfileSelectorsConflict(a.Selectors, m) { + a.Logger.Debugf("skipping subhelmfile %q: CLI selectors %v conflict with subhelmfile selectors %v", m.Path, a.Selectors, m.Selectors) continue } diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 804660c9..efcd8ab4 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -851,6 +851,89 @@ releases: runFilterSubHelmFilesTests(desiredTestcases, files, t, "2nd inherits") } +func TestSubhelmfileSelectorsConflict(t *testing.T) { + tests := []struct { + name string + cliSelectors []string + spec state.SubHelmfileSpec + conflict bool + }{ + { + name: "no CLI selectors", + cliSelectors: nil, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}}, + conflict: false, + }, + { + name: "no subhelmfile selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: nil}, + conflict: false, + }, + { + name: "selectorsInherited true", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}, SelectorsInherited: true}, + conflict: false, + }, + { + name: "conflicting selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}}, + conflict: true, + }, + { + name: "matching selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "b.yaml", Selectors: []string{"name=b"}}, + conflict: false, + }, + { + name: "different keys no conflict", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"env=prod"}}, + conflict: false, + }, + { + name: "empty CLI selectors slice", + cliSelectors: []string{}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}}, + conflict: false, + }, + { + name: "empty subhelmfile selectors slice", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{}}, + conflict: false, + }, + { + name: "all pairs conflict with multiple subhelmfile selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a", "name=c"}}, + conflict: true, + }, + { + name: "one matching pair among multiple subhelmfile selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a", "name=b"}}, + conflict: false, + }, + { + name: "selectorsInherited false with conflicting selectors still conflicts", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}, SelectorsInherited: false}, + conflict: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := subhelmfileSelectorsConflict(tt.cliSelectors, tt.spec) + assert.Equal(t, tt.conflict, got) + }) + } +} + func runFilterSubHelmFilesTests(testcases []struct { label string expectedReleases []string diff --git a/test/integration/test-cases/issue-2544.sh b/test/integration/test-cases/issue-2544.sh index 1851c73f..e42a82eb 100644 --- a/test/integration/test-cases/issue-2544.sh +++ b/test/integration/test-cases/issue-2544.sh @@ -13,27 +13,27 @@ trap cleanup_issue_2544 EXIT test_start "issue-2544: subhelmfiles skipped when selectors conflict with CLI" -# --- Test 1: -l name=b should only produce output from subhelmfile-b --------------------------- +# --- Test 1: -l app=b should only produce output from subhelmfile-b --------------------------- -info "Running helmfile template -l name=b" -${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l name=b \ +info "Running helmfile template -l app=b" +${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l app=b \ > "${issue_2544_tmp_dir}/output-b.txt" 2>&1 || { cat "${issue_2544_tmp_dir}/output-b.txt" - fail "helmfile template -l name=b failed" + fail "helmfile template -l app=b failed" } # Should contain release-b grep -q "release-b" "${issue_2544_tmp_dir}/output-b.txt" \ - || fail "output should contain release-b (matching selector name=b)" + || fail "output should contain release-b (matching selector app=b)" # Should NOT contain release-a or release-c (their subhelmfiles have incompatible selectors) grep -q "release-a" "${issue_2544_tmp_dir}/output-b.txt" \ - && fail "output should NOT contain release-a (subhelmfile selectors name=a conflict with name=b)" + && fail "output should NOT contain release-a (subhelmfile selectors app=a conflict with app=b)" grep -q "release-c" "${issue_2544_tmp_dir}/output-b.txt" \ - && fail "output should NOT contain release-c (subhelmfile selectors name=c conflict with name=b)" + && fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=b)" -info "PASS: -l name=b produces only release-b" +info "PASS: -l app=b produces only release-b" # --- Test 2: no selector should produce all releases ----------------------------------------- @@ -53,25 +53,25 @@ grep -q "release-c" "${issue_2544_tmp_dir}/output-all.txt" \ info "PASS: no selector produces all releases" -# --- Test 3: -l name=a should skip subhelmfiles with selectors name=b and name=c --------------- +# --- Test 3: -l app=a should skip subhelmfiles with selectors app=b and app=c --------------- -info "Running helmfile template -l name=a" -${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l name=a \ +info "Running helmfile template -l app=a" +${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l app=a \ > "${issue_2544_tmp_dir}/output-a.txt" 2>&1 || { cat "${issue_2544_tmp_dir}/output-a.txt" - fail "helmfile template -l name=a failed" + fail "helmfile template -l app=a failed" } grep -q "release-a" "${issue_2544_tmp_dir}/output-a.txt" \ - || fail "output should contain release-a (matching selector name=a)" + || fail "output should contain release-a (matching selector app=a)" grep -q "release-b" "${issue_2544_tmp_dir}/output-a.txt" \ - && fail "output should NOT contain release-b (subhelmfile selectors name=b conflict with name=a)" + && fail "output should NOT contain release-b (subhelmfile selectors app=b conflict with app=a)" grep -q "release-c" "${issue_2544_tmp_dir}/output-a.txt" \ - && fail "output should NOT contain release-c (subhelmfile selectors name=c conflict with name=a)" + && fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=a)" -info "PASS: -l name=a produces only release-a" +info "PASS: -l app=a produces only release-a" trap - EXIT test_pass "issue-2544: subhelmfiles skipped when selectors conflict with CLI" diff --git a/test/integration/test-cases/issue-2544/input/helmfile.yaml b/test/integration/test-cases/issue-2544/input/helmfile.yaml index 35659b60..ee171717 100644 --- a/test/integration/test-cases/issue-2544/input/helmfile.yaml +++ b/test/integration/test-cases/issue-2544/input/helmfile.yaml @@ -1,10 +1,10 @@ helmfiles: - path: subhelmfile-a.yaml selectors: - - name=a + - app=a - path: subhelmfile-b.yaml selectors: - - name=b + - app=b - path: subhelmfile-c.yaml selectors: - - name=c + - app=c diff --git a/test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml b/test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml index 91822849..e2e0acbb 100644 --- a/test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml +++ b/test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml @@ -2,7 +2,7 @@ releases: - name: release-a chart: ../../../charts/raw labels: - name: a + app: a values: - templates: - | diff --git a/test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml b/test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml index fc016f8b..882f4b40 100644 --- a/test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml +++ b/test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml @@ -2,7 +2,7 @@ releases: - name: release-b chart: ../../../charts/raw labels: - name: b + app: b values: - templates: - | diff --git a/test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml b/test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml index 9e93dc0d..933d0f8b 100644 --- a/test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml +++ b/test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml @@ -2,7 +2,7 @@ releases: - name: release-c chart: ../../../charts/raw labels: - name: c + app: c values: - templates: - |