From 6aeb6b38ba2d318b869b86e05182ec8bb02617f3 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 19 Sep 2022 04:31:45 +0000 Subject: [PATCH] Fix not to ignore diff selector when it matched nothing Fixes #327 Signed-off-by: Yusuke Kuoka --- pkg/app/app.go | 26 ++-- pkg/app/app_diff_test.go | 147 ++++++++++++++++++ .../show_diff_on_changed_selected_release | 40 +++++ ...ff_on_already_uninstalled_selected_release | 37 +++++ 4 files changed, 238 insertions(+), 12 deletions(-) create mode 100644 pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release create mode 100644 pkg/app/testdata/app_diff_test/shows_no_diff_on_already_uninstalled_selected_release diff --git a/pkg/app/app.go b/pkg/app/app.go index e4141468..88f18630 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1866,12 +1866,12 @@ func (a *App) withNeeds(r *Run, c DAGConfig, includeDisabled bool, f func(*state var toRender []state.ReleaseSpec - releasesDisabled := map[string]state.ReleaseSpec{} + releasesToUninstall := map[string]state.ReleaseSpec{} for _, r := range selectedReleasesWithNeeds { release := r id := state.ReleaseToID(&release) if release.Installed != nil && !*release.Installed { - releasesDisabled[id] = release + releasesToUninstall[id] = release } else { toRender = append(toRender, release) } @@ -1879,19 +1879,21 @@ func (a *App) withNeeds(r *Run, c DAGConfig, includeDisabled bool, f func(*state var rels []state.ReleaseSpec - // toRender already contains the direct and transitive needs depending on the DAG options. - // That's why we don't pass in `IncludeNeeds: c.IncludeNeeds(), IncludeTransitiveNeeds: c.IncludeTransitiveNeeds()` here. - // Otherwise, in case include-needs=true, it will include the needs of needs, which results in unexpectedly introducing transitive needs, - // even if include-transitive-needs=true is unspecified. - if _, errs := withDAG(st, r.helm, a.Logger, state.PlanOptions{SelectedReleases: toRender, Reverse: false, SkipNeeds: c.SkipNeeds(), IncludeNeeds: includeNeeds}, a.WrapWithoutSelector(func(subst *state.HelmState, helm helmexec.Interface) []error { - rels = append(rels, subst.Releases...) - return nil - })); len(errs) > 0 { - return false, errs + if len(toRender) > 0 { + // toRender already contains the direct and transitive needs depending on the DAG options. + // That's why we don't pass in `IncludeNeeds: c.IncludeNeeds(), IncludeTransitiveNeeds: c.IncludeTransitiveNeeds()` here. + // Otherwise, in case include-needs=true, it will include the needs of needs, which results in unexpectedly introducing transitive needs, + // even if include-transitive-needs=true is unspecified. + if _, errs := withDAG(st, r.helm, a.Logger, state.PlanOptions{SelectedReleases: toRender, Reverse: false, SkipNeeds: c.SkipNeeds(), IncludeNeeds: includeNeeds}, a.WrapWithoutSelector(func(subst *state.HelmState, helm helmexec.Interface) []error { + rels = append(rels, subst.Releases...) + return nil + })); len(errs) > 0 { + return false, errs + } } if includeDisabled { - for _, d := range releasesDisabled { + for _, d := range releasesToUninstall { rels = append(rels, d) } } diff --git a/pkg/app/app_diff_test.go b/pkg/app/app_diff_test.go index ffa70412..cee33363 100644 --- a/pkg/app/app_diff_test.go +++ b/pkg/app/app_diff_test.go @@ -321,3 +321,150 @@ releases: }) }) } + +func TestDiffWithInstalled(t *testing.T) { + type testcase struct { + helmfile string + ns string + error string + selectors []string + lists map[exectest.ListKey]string + diffed []exectest.Release + } + + check := func(t *testing.T, tc testcase) { + t.Helper() + + wantDiffs := tc.diffed + + var helm = &exectest.Helm{ + FailOnUnexpectedList: true, + FailOnUnexpectedDiff: true, + Lists: tc.lists, + DiffMutex: &sync.Mutex{}, + ChartsMutex: &sync.Mutex{}, + ReleasesMutex: &sync.Mutex{}, + } + + bs := &bytes.Buffer{} + + func() { + t.Helper() + + logReader, logWriter := io.Pipe() + + logFlushed := &sync.WaitGroup{} + // Ensure all the log is consumed into `bs` by calling `logWriter.Close()` followed by `logFlushed.Wait()` + logFlushed.Add(1) + go func() { + scanner := bufio.NewScanner(logReader) + for scanner.Scan() { + bs.Write(scanner.Bytes()) + bs.WriteString("\n") + } + logFlushed.Done() + }() + + defer func() { + // This is here to avoid data-trace on bytes buffer `bs` to capture logs + if err := logWriter.Close(); err != nil { + panic(err) + } + logFlushed.Wait() + }() + + logger := helmexec.NewLogger(logWriter, "debug") + + valsRuntime, err := vals.New(vals.Options{CacheSize: 32}) + if err != nil { + t.Errorf("unexpected error creating vals runtime: %v", err) + } + + files := map[string]string{ + "/path/to/helmfile.yaml": tc.helmfile, + } + + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + fs: filesystem.DefaultFileSystem(), + OverrideKubeContext: "default", + Env: "default", + Logger: logger, + helms: map[helmKey]helmexec.Interface{ + createHelmKey("helm", "default"): helm, + }, + valsRuntime: valsRuntime, + }, files) + + if tc.ns != "" { + app.Namespace = tc.ns + } + + if tc.selectors != nil { + app.Selectors = tc.selectors + } + + diffErr := app.Diff(applyConfig{ + // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. + concurrency: 1, + logger: logger, + skipNeeds: true, + }) + + var gotErr string + if diffErr != nil { + gotErr = diffErr.Error() + } + + if d := cmp.Diff(tc.error, gotErr); d != "" { + t.Fatalf("unexpected error: want (-), got (+): %s", d) + } + + require.Equal(t, wantDiffs, helm.Diffed) + }() + + testhelper.RequireLog(t, "app_diff_test", bs) + } + + t.Run("show diff on changed selected release", func(t *testing.T) { + check(t, testcase{ + helmfile: ` +releases: +- name: a + chart: incubator/raw + namespace: default +- name: b + chart: incubator/raw + namespace: default +`, + selectors: []string{"name=a"}, + lists: map[exectest.ListKey]string{ + {Filter: "^a$", Flags: helmV2ListFlags}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +foo 4 Fri Nov 1 08:40:07 2019 DEPLOYED raw-3.1.0 3.1.0 default +`, + }, + diffed: []exectest.Release{ + {Name: "a", Flags: []string{"--kube-context", "default", "--namespace", "default"}}, + }, + }) + }) + + t.Run("shows no diff on already uninstalled selected release", func(t *testing.T) { + check(t, testcase{ + helmfile: ` +releases: +- name: a + chart: incubator/raw + installed: false + namespace: default +- name: b + chart: incubator/raw + namespace: default +`, + selectors: []string{"name=a"}, + lists: map[exectest.ListKey]string{ + {Filter: "^a$", Flags: helmV2ListFlags}: ``, + }, + }) + }) +} diff --git a/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release b/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release new file mode 100644 index 00000000..466548bf --- /dev/null +++ b/pkg/app/testdata/app_diff_test/show_diff_on_changed_selected_release @@ -0,0 +1,40 @@ +processing file "helmfile.yaml" in directory "." +changing working directory to "/path/to" +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: a + 3: chart: incubator/raw + 4: namespace: default + 5: - name: b + 6: chart: incubator/raw + 7: namespace: default + 8: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: a + 3: chart: incubator/raw + 4: namespace: default + 5: - name: b + 6: chart: incubator/raw + 7: namespace: default + 8: + +merged environment: &{default map[] map[]} +1 release(s) matching name=a found in helmfile.yaml + +processing 1 groups of releases in this order: +GROUP RELEASES +1 default/default/a + +processing releases in group 1/1: default/default/a +changing working directory back to "/path/to" diff --git a/pkg/app/testdata/app_diff_test/shows_no_diff_on_already_uninstalled_selected_release b/pkg/app/testdata/app_diff_test/shows_no_diff_on_already_uninstalled_selected_release new file mode 100644 index 00000000..2d283c5c --- /dev/null +++ b/pkg/app/testdata/app_diff_test/shows_no_diff_on_already_uninstalled_selected_release @@ -0,0 +1,37 @@ +processing file "helmfile.yaml" in directory "." +changing working directory to "/path/to" +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: a + 3: chart: incubator/raw + 4: installed: false + 5: namespace: default + 6: - name: b + 7: chart: incubator/raw + 8: namespace: default + 9: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: a + 3: chart: incubator/raw + 4: installed: false + 5: namespace: default + 6: - name: b + 7: chart: incubator/raw + 8: namespace: default + 9: + +merged environment: &{default map[] map[]} +1 release(s) matching name=a found in helmfile.yaml + +changing working directory back to "/path/to"