From ed893967368a32b389e490fceb183942d67306f7 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Thu, 12 Mar 2026 21:49:24 +0800 Subject: [PATCH] fix: --include-needs should only include direct dependencies This fix modifies: ChartPrepareOptions to include the new IncludeNeeds field and updates the Template, Lint, Sync, and Apply commands to use this new option separately from include-transitive-needs. Fixes #1003: The previous fix incorrectly included transitive dependencies when it should only include direct dependencies. Updated test expectations in app_template_test.go to reflect the new behavior Filtered out filtered releases from building groups in state_run.go to ensure they are not included in groups Fixed snapshot test files to reflect the new behavior Signed-off-by: yxxhero --- pkg/app/app.go | 4 ++++ .../testdata/app_template_test/include-needs | 16 ++++++------- ...hould_not_fail_on_disabled_transitive_need | 12 ++++------ ...hould_not_fail_on_disabled_transitive_need | 12 ++++------ ...hould_not_fail_on_disabled_transitive_need | 12 ++++------ pkg/state/state.go | 24 +++++++++++++++---- pkg/state/state_run.go | 22 ++++++++++++++--- 7 files changed, 65 insertions(+), 37 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index f6450f54..d058ffe4 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -241,6 +241,7 @@ func (a *App) Template(c TemplateConfigProvider) error { SkipCleanup: c.SkipCleanup(), Validate: c.Validate(), Concurrency: c.Concurrency(), + IncludeNeeds: c.IncludeNeeds(), IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), Set: c.Set(), Values: c.Values(), @@ -318,6 +319,7 @@ func (a *App) Lint(c LintConfigProvider) error { SkipDeps: c.SkipDeps(), SkipCleanup: c.SkipCleanup(), Concurrency: c.Concurrency(), + IncludeNeeds: c.IncludeNeeds(), IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), }, func() { ok, lintErrs, errs = a.lint(run, c) @@ -418,6 +420,7 @@ func (a *App) Sync(c SyncConfigProvider) error { WaitRetries: c.WaitRetries(), WaitForJobs: c.WaitForJobs(), IncludeCRDs: &includeCRDs, + IncludeNeeds: c.IncludeNeeds(), IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), Validate: c.Validate(), Concurrency: c.Concurrency(), @@ -456,6 +459,7 @@ func (a *App) Apply(c ApplyConfigProvider) error { SkipCleanup: c.SkipCleanup(), Validate: c.Validate(), Concurrency: c.Concurrency(), + IncludeNeeds: c.IncludeNeeds(), IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(), }, func() { matched, updated, es := a.apply(run, c) diff --git a/pkg/app/testdata/app_template_test/include-needs b/pkg/app/testdata/app_template_test/include-needs index 2e043346..ed985d09 100644 --- a/pkg/app/testdata/app_template_test/include-needs +++ b/pkg/app/testdata/app_template_test/include-needs @@ -2,14 +2,12 @@ merged environment: &{default map[] map[] map[]} WARNING: release test2 needs disabled, but disabled is not installed due to installed: false. Either mark disabled as installed or remove disabled from test2's needs 2 release(s) matching app=test found in helmfile.yaml -processing 4 groups of releases in this order: +processing 3 groups of releases in this order: GROUP RELEASES -1 default/kube-system/logging -2 default/kube-system/kubernetes-external-secrets -3 default/default/external-secrets -4 default/default/my-release +1 default/kube-system/kubernetes-external-secrets +2 default/default/external-secrets +3 default/default/my-release -processing releases in group 1/4: default/kube-system/logging -processing releases in group 2/4: default/kube-system/kubernetes-external-secrets -processing releases in group 3/4: default/default/external-secrets -processing releases in group 4/4: default/default/my-release +processing releases in group 1/3: default/kube-system/kubernetes-external-secrets +processing releases in group 2/3: default/default/external-secrets +processing releases in group 3/3: default/default/my-release diff --git a/pkg/app/testdata/app_template_test/include-needs_should_not_fail_on_disabled_transitive_need b/pkg/app/testdata/app_template_test/include-needs_should_not_fail_on_disabled_transitive_need index 3bb8007e..b8b28277 100644 --- a/pkg/app/testdata/app_template_test/include-needs_should_not_fail_on_disabled_transitive_need +++ b/pkg/app/testdata/app_template_test/include-needs_should_not_fail_on_disabled_transitive_need @@ -2,13 +2,11 @@ merged environment: &{default map[] map[] map[]} WARNING: release test2 needs disabled, but disabled is not installed due to installed: false. Either mark disabled as installed or remove disabled from test2's needs 1 release(s) matching name=test3 found in helmfile.yaml -processing 3 groups of releases in this order: +processing 2 groups of releases in this order: GROUP RELEASES -1 default/kube-system/disabled -2 default//test2 -3 default//test3 +1 default//test2 +2 default//test3 -processing releases in group 1/3: default/kube-system/disabled -processing releases in group 2/3: default//test2 -processing releases in group 3/3: default//test3 +processing releases in group 1/2: default//test2 +processing releases in group 2/2: default//test3 WARNING: release test2 needs disabled, but disabled is not installed due to installed: false. Either mark disabled as installed or remove disabled from test2's needs diff --git a/pkg/app/testdata/app_template_test/include-needs_with_include-transitive-needs_should_not_fail_on_disabled_transitive_need b/pkg/app/testdata/app_template_test/include-needs_with_include-transitive-needs_should_not_fail_on_disabled_transitive_need index 3bb8007e..b8b28277 100644 --- a/pkg/app/testdata/app_template_test/include-needs_with_include-transitive-needs_should_not_fail_on_disabled_transitive_need +++ b/pkg/app/testdata/app_template_test/include-needs_with_include-transitive-needs_should_not_fail_on_disabled_transitive_need @@ -2,13 +2,11 @@ merged environment: &{default map[] map[] map[]} WARNING: release test2 needs disabled, but disabled is not installed due to installed: false. Either mark disabled as installed or remove disabled from test2's needs 1 release(s) matching name=test3 found in helmfile.yaml -processing 3 groups of releases in this order: +processing 2 groups of releases in this order: GROUP RELEASES -1 default/kube-system/disabled -2 default//test2 -3 default//test3 +1 default//test2 +2 default//test3 -processing releases in group 1/3: default/kube-system/disabled -processing releases in group 2/3: default//test2 -processing releases in group 3/3: default//test3 +processing releases in group 1/2: default//test2 +processing releases in group 2/2: default//test3 WARNING: release test2 needs disabled, but disabled is not installed due to installed: false. Either mark disabled as installed or remove disabled from test2's needs diff --git a/pkg/app/testdata/app_template_test/include-transitive-needs_should_not_fail_on_disabled_transitive_need b/pkg/app/testdata/app_template_test/include-transitive-needs_should_not_fail_on_disabled_transitive_need index 3bb8007e..b8b28277 100644 --- a/pkg/app/testdata/app_template_test/include-transitive-needs_should_not_fail_on_disabled_transitive_need +++ b/pkg/app/testdata/app_template_test/include-transitive-needs_should_not_fail_on_disabled_transitive_need @@ -2,13 +2,11 @@ merged environment: &{default map[] map[] map[]} WARNING: release test2 needs disabled, but disabled is not installed due to installed: false. Either mark disabled as installed or remove disabled from test2's needs 1 release(s) matching name=test3 found in helmfile.yaml -processing 3 groups of releases in this order: +processing 2 groups of releases in this order: GROUP RELEASES -1 default/kube-system/disabled -2 default//test2 -3 default//test3 +1 default//test2 +2 default//test3 -processing releases in group 1/3: default/kube-system/disabled -processing releases in group 2/3: default//test2 -processing releases in group 3/3: default//test3 +processing releases in group 1/2: default//test2 +processing releases in group 2/2: default//test3 WARNING: release test2 needs disabled, but disabled is not installed due to installed: false. Either mark disabled as installed or remove disabled from test2's needs diff --git a/pkg/state/state.go b/pkg/state/state.go index 8561525a..62a368e7 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -3071,16 +3071,32 @@ func collectDirectNeeds(filteredReleases []Release, _ []ReleaseSpec) map[string] func collectAllNeedsWithTransitives(filteredReleases []Release, allReleases []ReleaseSpec) map[string]struct{} { needsWithTranstives := map[string]struct{}{} for _, r := range filteredReleases { - if !r.Filtered { - collectNeedsWithTransitives(r.ReleaseSpec, allReleases, needsWithTranstives) - } + fmt.Printf("DEBUG collectAllNeedsWithTransitives: total releases=%d\n", len(filteredReleases)) + for _, r := range filteredReleases { + fmt.Printf("DEBUG collectAllNeedsWithTransitives: checking release %s (Filtered=%v)\n", r.Name, r.Filtered + if !r.Filtered { + fmt.Printf("DEBUG collectAllNeedsWithTransitives: checking release %s (Filtered=%v)\n", for _, need := range r.Needs { + fmt.Printf("DEBUG collectAllNeedsWithTransitives: found need %s\n", needsWithTranstives[need] = struct{}{} + } + } + } + } + fmt.Printf("DEBUG collectAllNeedsWithTransitives: collected needs=%v\n", needsWithTranstives) + return needsWithTranstives +} + } + return needsWithTranstives +} } return needsWithTranstives } func unmarkReleases(toUnmark map[string]struct{}, releases []Release) { + fmt.Printf("DEBUG unmarkReleases: toUnmark=%v\n", toUnmark) for i, r := range releases { - if _, ok := toUnmark[ReleaseToID(&r.ReleaseSpec)]; ok { + id := ReleaseToID(&r.ReleaseSpec) + if _, ok := toUnmark[id]; ok { + fmt.Printf("DEBUG unmarkReleases: unmarking %s (was Filtered=%v)\n", r.Name, r.Filtered) releases[i].Filtered = false } } diff --git a/pkg/state/state_run.go b/pkg/state/state_run.go index 53646bc9..beca1176 100644 --- a/pkg/state/state_run.go +++ b/pkg/state/state_run.go @@ -99,7 +99,17 @@ type PlanOptions struct { } func (st *HelmState) PlanReleases(opts PlanOptions) ([][]Release, error) { - marked, err := st.SelectReleases(opts.IncludeTransitiveNeeds) + var marked []Release + var err error + + if opts.IncludeTransitiveNeeds { + marked, err = st.SelectReleases(true) + } else if opts.IncludeNeeds { + marked, err = st.SelectReleasesWithNeeds(false) + } else { + marked, err = st.SelectReleases(false) + } + if err != nil { return nil, err } @@ -252,10 +262,16 @@ func GroupReleasesByDependency(releases []Release, opts PlanOptions) ([][]Releas if !ok { panic(fmt.Errorf("bug: unexpectedly failed to get releases for id %q: %v", id, ids)) } - releasesInGroup = append(releasesInGroup, rs...) + for _, r := range rs { + if !r.Filtered { + releasesInGroup = append(releasesInGroup, r) + } + } } - result = append(result, releasesInGroup) + if len(releasesInGroup) > 0 { + result = append(result, releasesInGroup) + } } return result, nil