diff --git a/pkg/app/app.go b/pkg/app/app.go index 232a67c0..83ab7880 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1095,14 +1095,41 @@ func (a *App) findDesiredStateFiles(specifiedPath string, opts LoadOpts) ([]stri return files, nil } -func (a *App) getSelectedReleases(r *Run) ([]state.ReleaseSpec, error) { - releases, err := r.state.GetSelectedReleasesWithOverrides() +func (a *App) getSelectedReleases(r *Run) ([]state.ReleaseSpec, []state.ReleaseSpec, error) { + selected, err := r.state.GetSelectedReleasesWithOverrides() if err != nil { - return nil, err + return nil, nil, err + } + + selectedIds := map[string]struct{}{} + for _, r := range selected { + selectedIds[state.ReleaseToID(&r)] = struct{}{} + } + + allReleases := r.state.GetReleasesWithOverrides() + + needed := map[string]struct{}{} + for _, r := range selected { + for _, id := range r.Needs { + // Avoids duplicating a release that is selected AND also needed by another selected release + if _, ok := selectedIds[id]; !ok { + needed[id] = struct{}{} + } + } + } + + var releases []state.ReleaseSpec + + releases = append(releases, selected...) + + for _, r := range allReleases { + if _, ok := needed[state.ReleaseToID(&r)]; ok { + releases = append(releases, r) + } } if err := checkDuplicates(r.helm, r.state, releases); err != nil { - return nil, err + return nil, nil, err } var extra string @@ -1111,24 +1138,30 @@ func (a *App) getSelectedReleases(r *Run) ([]state.ReleaseSpec, error) { extra = " matching " + strings.Join(r.state.Selectors, ",") } - a.Logger.Debugf("%d release(s)%s found in %s\n", len(releases), extra, r.state.FilePath) + a.Logger.Debugf("%d release(s)%s found in %s\n", len(selected), extra, r.state.FilePath) - return releases, nil + return selected, releases, nil } func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { st := r.state helm := r.helm - toApply, err := a.getSelectedReleases(r) + selectedReleases, selectedAndNeededReleases, err := a.getSelectedReleases(r) if err != nil { return false, false, []error{err} } - if len(toApply) == 0 { + if len(selectedReleases) == 0 { return false, false, nil } - plan, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: toApply, SkipNeeds: c.SkipNeeds(), IncludeNeeds: c.IncludeNeeds()}) + // This is required when you're trying to deduplicate releases by the selector. + // Without this, `PlanReleases` conflates duplicates and return both in `batches`, + // even if we provided `SelectedReleases: selectedReleases`. + // See https://github.com/roboll/helmfile/issues/1818 for more context. + st.Releases = selectedAndNeededReleases + + plan, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: selectedReleases, SkipNeeds: c.SkipNeeds(), IncludeNeeds: c.IncludeNeeds()}) if err != nil { return false, false, []error{err} } @@ -1275,7 +1308,7 @@ func (a *App) delete(r *Run, purge bool, c DestroyConfigProvider) (bool, []error affectedReleases := state.AffectedReleases{} - toSync, err := a.getSelectedReleases(r) + toSync, _, err := a.getSelectedReleases(r) if err != nil { return false, []error{err} } @@ -1315,6 +1348,8 @@ func (a *App) delete(r *Run, purge bool, c DestroyConfigProvider) (bool, []error names[i] = fmt.Sprintf(" %s (%s)", r.Name, r.Chart) } + st.Releases = st.GetReleasesWithOverrides() + var errs []error msg := fmt.Sprintf(`Affected releases are: @@ -1329,17 +1364,7 @@ Do you really want to delete? r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) if len(releasesToDelete) > 0 { - _, deletionErrs := withDAG(st, helm, a.Logger, state.PlanOptions{Reverse: true, SkipNeeds: true}, a.Wrap(func(subst *state.HelmState, helm helmexec.Interface) []error { - var rs []state.ReleaseSpec - - for _, r := range subst.Releases { - if _, ok := releasesToDelete[state.ReleaseToID(&r)]; ok { - rs = append(rs, r) - } - } - - subst.Releases = rs - + _, deletionErrs := withDAG(st, helm, a.Logger, state.PlanOptions{SelectedReleases: toDelete, Reverse: true, SkipNeeds: true}, a.WrapWithoutSelector(func(subst *state.HelmState, helm helmexec.Interface) []error { return subst.DeleteReleases(&affectedReleases, helm, c.Concurrency(), purge) })) @@ -1355,12 +1380,12 @@ Do you really want to delete? func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) { st := r.state - toDiff, err := a.getSelectedReleases(r) + selectedReleases, selectedAndNeededReleases, err := a.getSelectedReleases(r) if err != nil { return nil, false, false, []error{err} } - if len(toDiff) == 0 { + if len(selectedReleases) == 0 { return nil, false, false, nil } @@ -1373,7 +1398,9 @@ func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) Set: c.Set(), } - plan, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: toDiff, SkipNeeds: c.SkipNeeds(), IncludeNeeds: c.IncludeNeeds()}) + st.Releases = selectedAndNeededReleases + + plan, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: selectedReleases, SkipNeeds: c.SkipNeeds(), IncludeNeeds: c.IncludeNeeds()}) if err != nil { return nil, false, false, []error{err} } @@ -1408,7 +1435,7 @@ func (a *App) lint(r *Run, c LintConfigProvider) (bool, []error) { allReleases := st.GetReleasesWithOverrides() - selectedReleases, err := a.getSelectedReleases(r) + selectedReleases, _, err := a.getSelectedReleases(r) if err != nil { return false, []error{err} } @@ -1474,7 +1501,7 @@ func (a *App) status(r *Run, c StatusesConfigProvider) (bool, []error) { allReleases := st.GetReleasesWithOverrides() - selectedReleases, err := a.getSelectedReleases(r) + selectedReleases, selectedAndNeededReleases, err := a.getSelectedReleases(r) if err != nil { return false, []error{err} } @@ -1484,7 +1511,7 @@ func (a *App) status(r *Run, c StatusesConfigProvider) (bool, []error) { // Do build deps and prepare only on selected releases so that we won't waste time // on running various helm commands on unnecessary releases - st.Releases = selectedReleases + st.Releases = selectedAndNeededReleases releasesToRender := map[string]state.ReleaseSpec{} for _, r := range selectedReleases { @@ -1535,15 +1562,21 @@ func (a *App) sync(r *Run, c SyncConfigProvider) (bool, []error) { st := r.state helm := r.helm - toSync, err := a.getSelectedReleases(r) + selectedReleases, selectedAndNeededReleases, err := a.getSelectedReleases(r) if err != nil { return false, []error{err} } - if len(toSync) == 0 { + if len(selectedReleases) == 0 { return false, nil } - batches, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: toSync, IncludeNeeds: c.IncludeNeeds(), SkipNeeds: c.SkipNeeds()}) + // This is required when you're trying to deduplicate releases by the selector. + // Without this, `PlanReleases` conflates duplicates and return both in `batches`, + // even if we provided `SelectedReleases: selectedReleases`. + // See https://github.com/roboll/helmfile/issues/1818 for more context. + st.Releases = selectedAndNeededReleases + + batches, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: selectedReleases, IncludeNeeds: c.IncludeNeeds(), SkipNeeds: c.SkipNeeds()}) if err != nil { return false, []error{err} } @@ -1685,7 +1718,7 @@ func (a *App) template(r *Run, c TemplateConfigProvider) (bool, []error) { st := r.state helm := r.helm - selectedReleases, err := a.getSelectedReleases(r) + selectedReleases, selectedAndNeededReleases, err := a.getSelectedReleases(r) if err != nil { return false, []error{err} } @@ -1693,6 +1726,12 @@ func (a *App) template(r *Run, c TemplateConfigProvider) (bool, []error) { return false, nil } + // This is required when you're trying to deduplicate releases by the selector. + // Without this, `PlanReleases` conflates duplicates and return both in `batches`, + // even if we provided `SelectedReleases: selectedReleases`. + // See https://github.com/roboll/helmfile/issues/1818 for more context. + st.Releases = selectedAndNeededReleases + batches, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: selectedReleases, IncludeNeeds: c.IncludeNeeds(), SkipNeeds: !c.IncludeNeeds()}) if err != nil { return false, []error{err} @@ -1757,7 +1796,7 @@ func (a *App) test(r *Run, c TestConfigProvider) []error { st := r.state - toTest, err := a.getSelectedReleases(r) + toTest, _, err := a.getSelectedReleases(r) if err != nil { return []error{err} } @@ -1779,7 +1818,7 @@ func (a *App) writeValues(r *Run, c WriteValuesConfigProvider) (bool, []error) { st := r.state helm := r.helm - toRender, err := a.getSelectedReleases(r) + toRender, _, err := a.getSelectedReleases(r) if err != nil { return false, []error{err} } diff --git a/pkg/app/app_apply_test.go b/pkg/app/app_apply_test.go index 59c2bfbb..7a16a907 100644 --- a/pkg/app/app_apply_test.go +++ b/pkg/app/app_apply_test.go @@ -163,6 +163,8 @@ func TestApply_2(t *testing.T) { if exists { t.Errorf("unexpected log:\nDIFF\n%s\nEOD", diff) } + } else { + assertEqualsToSnapshot(t, "log", bs.String()) } } @@ -1061,4 +1063,37 @@ merged environment: &{default map[] map[]} `, }) }) + + t.Run("deduplicate by --selector", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + component: raw + index: '1' + +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + component: raw + index: '2' +`, + }, + selectors: []string{"index=1"}, + upgraded: []exectest.Release{}, + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + }) + }) } diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index b5e4e37f..d66b2e18 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -2979,30 +2979,30 @@ Affected releases are: processing 2 groups of releases in this order: GROUP RELEASES -1 default/frontend-v1 -2 default/backend-v1 +1 default//frontend-v1 +2 default//backend-v1 -processing releases in group 1/2: default/frontend-v1 -processing releases in group 2/2: default/backend-v1 +processing releases in group 1/2: default//frontend-v1 +processing releases in group 2/2: default//backend-v1 processing 5 groups of releases in this order: GROUP RELEASES -1 default/logging, default/front-proxy -2 default/database, default/servicemesh -3 default/anotherbackend -4 default/backend-v2 -5 default/frontend-v3 +1 default//logging, default//front-proxy +2 default//database, default//servicemesh +3 default//anotherbackend +4 default//backend-v2 +5 default//frontend-v3 -processing releases in group 1/5: default/logging, default/front-proxy +processing releases in group 1/5: default//logging, default//front-proxy getting deployed release version failed:unexpected list key: {^logging$ --kube-contextdefault--deleting--deployed--failed--pending} getting deployed release version failed:unexpected list key: {^front-proxy$ --kube-contextdefault--deleting--deployed--failed--pending} -processing releases in group 2/5: default/database, default/servicemesh +processing releases in group 2/5: default//database, default//servicemesh getting deployed release version failed:unexpected list key: {^database$ --kube-contextdefault--deleting--deployed--failed--pending} getting deployed release version failed:unexpected list key: {^servicemesh$ --kube-contextdefault--deleting--deployed--failed--pending} -processing releases in group 3/5: default/anotherbackend +processing releases in group 3/5: default//anotherbackend getting deployed release version failed:unexpected list key: {^anotherbackend$ --kube-contextdefault--deleting--deployed--failed--pending} -processing releases in group 4/5: default/backend-v2 +processing releases in group 4/5: default//backend-v2 getting deployed release version failed:unexpected list key: {^backend-v2$ --kube-contextdefault--deleting--deployed--failed--pending} -processing releases in group 5/5: default/frontend-v3 +processing releases in group 5/5: default//frontend-v3 getting deployed release version failed:unexpected list key: {^frontend-v3$ --kube-contextdefault--deleting--deployed--failed--pending} UPDATED RELEASES: @@ -3128,13 +3128,13 @@ Affected releases are: processing 2 groups of releases in this order: GROUP RELEASES -1 default/baz, default/bar -2 default/foo +1 default//baz, default//bar +2 default//foo -processing releases in group 1/2: default/baz, default/bar +processing releases in group 1/2: default//baz, default//bar getting deployed release version failed:unexpected list key: {^baz$ --kube-contextdefault--deleting--deployed--failed--pending} getting deployed release version failed:unexpected list key: {^bar$ --kube-contextdefault--deleting--deployed--failed--pending} -processing releases in group 2/2: default/foo +processing releases in group 2/2: default//foo getting deployed release version failed:unexpected list key: {^foo$ --kube-contextdefault--deleting--deployed--failed--pending} UPDATED RELEASES: @@ -3238,11 +3238,11 @@ Affected releases are: processing 2 groups of releases in this order: GROUP RELEASES -1 default/baz, default/bar -2 default/foo +1 default//baz, default//bar +2 default//foo -processing releases in group 1/2: default/baz, default/bar -processing releases in group 2/2: default/foo +processing releases in group 1/2: default//baz, default//bar +processing releases in group 2/2: default//foo getting deployed release version failed:Failed to get the version for:mychart1 UPDATED RELEASES: @@ -3346,11 +3346,11 @@ Affected releases are: processing 2 groups of releases in this order: GROUP RELEASES -1 default/baz, default/bar -2 default/foo +1 default//baz, default//bar +2 default//foo -processing releases in group 1/2: default/baz, default/bar -processing releases in group 2/2: default/foo +processing releases in group 1/2: default//baz, default//bar +processing releases in group 2/2: default//foo getting deployed release version failed:Failed to get the version for:mychart1 UPDATED RELEASES: @@ -3509,7 +3509,7 @@ releases: }, }, { - name: "upgrade when tns1/ns1/foo needs tns2/ns2/bar", + name: "helm2: upgrade when tns1/foo needs tns2/bar", loc: location(), files: map[string]string{ @@ -3520,7 +3520,7 @@ releases: namespace: ns1 tillerNamespace: tns1 needs: - - tns2/ns2/bar + - tns2/bar - name: bar chart: stable/mychart2 namespace: ns2 @@ -3537,7 +3537,7 @@ releases: }, }, { - name: "upgrade when tns2/ns2/bar needs tns1/ns1/foo", + name: "helm2: upgrade when tns2/bar needs tns1/foo", loc: location(), files: map[string]string{ "/path/to/helmfile.yaml": ` @@ -3547,7 +3547,7 @@ releases: namespace: ns2 tillerNamespace: tns2 needs: - - tns1/ns1/foo + - tns1/foo - name: foo chart: stable/mychart1 namespace: ns1 @@ -3575,7 +3575,7 @@ first-pass rendering output of "helmfile.yaml.part.0": 4: namespace: ns2 5: tillerNamespace: tns2 6: needs: - 7: - tns1/ns1/foo + 7: - tns1/foo 8: - name: foo 9: chart: stable/mychart1 10: namespace: ns1 @@ -3595,7 +3595,7 @@ second-pass rendering result of "helmfile.yaml.part.0": 4: namespace: ns2 5: tillerNamespace: tns2 6: needs: - 7: - tns1/ns1/foo + 7: - tns1/foo 8: - name: foo 9: chart: stable/mychart1 10: namespace: ns1 @@ -3611,12 +3611,12 @@ Affected releases are: processing 2 groups of releases in this order: GROUP RELEASES -1 default/tns1/ns1/foo -2 default/tns2/ns2/bar +1 default/tns1/foo +2 default/tns2/bar -processing releases in group 1/2: default/tns1/ns1/foo +processing releases in group 1/2: default/tns1/foo getting deployed release version failed:unexpected list key: {^foo$ --tiller-namespacetns1--kube-contextdefault--deleting--deployed--failed--pending} -processing releases in group 2/2: default/tns2/ns2/bar +processing releases in group 2/2: default/tns2/bar getting deployed release version failed:unexpected list key: {^bar$ --tiller-namespacetns2--kube-contextdefault--deleting--deployed--failed--pending} UPDATED RELEASES: @@ -4201,7 +4201,7 @@ releases: upgraded: []exectest.Release{}, deleted: []exectest.Release{}, concurrency: 1, - error: `in ./helmfile.yaml: release "default/foo" depends on "default/bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, + error: `in ./helmfile.yaml: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, log: `processing file "helmfile.yaml" in directory "." first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= first-pass uses: &{default map[] map[]} @@ -4237,7 +4237,7 @@ second-pass rendering result of "helmfile.yaml.part.0": merged environment: &{default map[] map[]} 2 release(s) found in helmfile.yaml -err: release "default/foo" depends on "default/bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies +err: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies `, }, } diff --git a/pkg/app/destroy_test.go b/pkg/app/destroy_test.go index d9b0f132..ac969e69 100644 --- a/pkg/app/destroy_test.go +++ b/pkg/app/destroy_test.go @@ -442,25 +442,25 @@ merged environment: &{default map[] map[]} processing 5 groups of releases in this order: GROUP RELEASES -1 default/frontend-v3, default/frontend-v2, default/frontend-v1 -2 default/backend-v2, default/backend-v1 -3 default/anotherbackend -4 default/servicemesh, default/database -5 default/front-proxy, default/logging +1 default//frontend-v3, default//frontend-v2, default//frontend-v1 +2 default//backend-v2, default//backend-v1 +3 default//anotherbackend +4 default//servicemesh, default//database +5 default//front-proxy, default//logging -processing releases in group 1/5: default/frontend-v3, default/frontend-v2, default/frontend-v1 +processing releases in group 1/5: default//frontend-v3, default//frontend-v2, default//frontend-v1 release "frontend-v3" processed release "frontend-v2" processed release "frontend-v1" processed -processing releases in group 2/5: default/backend-v2, default/backend-v1 +processing releases in group 2/5: default//backend-v2, default//backend-v1 release "backend-v2" processed release "backend-v1" processed -processing releases in group 3/5: default/anotherbackend +processing releases in group 3/5: default//anotherbackend release "anotherbackend" processed -processing releases in group 4/5: default/servicemesh, default/database +processing releases in group 4/5: default//servicemesh, default//database release "servicemesh" processed release "database" processed -processing releases in group 5/5: default/front-proxy, default/logging +processing releases in group 5/5: default//front-proxy, default//logging release "front-proxy" processed release "logging" processed @@ -648,9 +648,9 @@ merged environment: &{default map[] map[]} processing 1 groups of releases in this order: GROUP RELEASES -1 default/logging +1 default//logging -processing releases in group 1/1: default/logging +processing releases in group 1/1: default//logging release "logging" processed DELETED RELEASES: @@ -713,12 +713,12 @@ merged environment: &{default map[] map[]} processing 2 groups of releases in this order: GROUP RELEASES -1 default/frontend-v1 -2 default/backend-v1 +1 default//frontend-v1 +2 default//backend-v1 -processing releases in group 1/2: default/frontend-v1 +processing releases in group 1/2: default//frontend-v1 release "frontend-v1" processed -processing releases in group 2/2: default/backend-v1 +processing releases in group 2/2: default//backend-v1 release "backend-v1" processed DELETED RELEASES: @@ -783,12 +783,12 @@ merged environment: &{default map[] map[]} processing 2 groups of releases in this order: GROUP RELEASES -1 default/frontend-v1 -2 default/backend-v1 +1 default//frontend-v1 +2 default//backend-v1 -processing releases in group 1/2: default/frontend-v1 +processing releases in group 1/2: default//frontend-v1 release "frontend-v1" processed -processing releases in group 2/2: default/backend-v1 +processing releases in group 2/2: default//backend-v1 release "backend-v1" processed DELETED RELEASES: diff --git a/pkg/app/diff_nokubectx_test.go b/pkg/app/diff_nokubectx_test.go index 254d43c7..83e9dc14 100644 --- a/pkg/app/diff_nokubectx_test.go +++ b/pkg/app/diff_nokubectx_test.go @@ -513,7 +513,7 @@ releases: upgraded: []exectest.Release{}, }, { - name: "upgrade when tns1/ns1/foo needs tns2/ns2/bar", + name: "helm2: upgrade when tns1/foo needs tns2/bar", loc: location(), files: map[string]string{ @@ -524,7 +524,7 @@ releases: namespace: ns1 tillerNamespace: tns1 needs: - - tns2/ns2/bar + - tns2/bar - name: bar chart: mychart2 namespace: ns2 @@ -540,7 +540,7 @@ releases: upgraded: []exectest.Release{}, }, { - name: "upgrade when tns2/ns2/bar needs tns1/ns1/foo", + name: "helm2: upgrade when tns2/bar needs tns1/foo", loc: location(), files: map[string]string{ "/path/to/helmfile.yaml": ` @@ -550,7 +550,7 @@ releases: namespace: ns2 tillerNamespace: tns2 needs: - - tns1/ns1/foo + - tns1/foo - name: foo chart: mychart1 namespace: ns1 @@ -577,7 +577,7 @@ first-pass rendering output of "helmfile.yaml.part.0": 4: namespace: ns2 5: tillerNamespace: tns2 6: needs: - 7: - tns1/ns1/foo + 7: - tns1/foo 8: - name: foo 9: chart: mychart1 10: namespace: ns1 @@ -597,7 +597,7 @@ second-pass rendering result of "helmfile.yaml.part.0": 4: namespace: ns2 5: tillerNamespace: tns2 6: needs: - 7: - tns1/ns1/foo + 7: - tns1/foo 8: - name: foo 9: chart: mychart1 10: namespace: ns1 @@ -607,6 +607,74 @@ second-pass rendering result of "helmfile.yaml.part.0": merged environment: &{default map[] map[]} 2 release(s) found in helmfile.yaml +Affected releases are: + bar (mychart2) UPDATED + foo (mychart1) UPDATED + +`, + }, + { + name: "helm3: upgrade when ns2/bar needs ns1/foo", + loc: location(), + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: bar + chart: mychart2 + namespace: ns2 + needs: + - ns1/foo +- name: foo + chart: mychart1 + namespace: ns1 +`, + }, + detailedExitcode: true, + error: "Identified at least one change", + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "bar", Chart: "mychart2", Flags: "--namespacens2--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "foo", Chart: "mychart1", Flags: "--namespacens1--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + upgraded: []exectest.Release{}, + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + log: `processing file "helmfile.yaml" in directory "." +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: bar + 3: chart: mychart2 + 4: namespace: ns2 + 5: needs: + 6: - ns1/foo + 7: - name: foo + 8: chart: mychart1 + 9: namespace: ns1 +10: + +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: bar + 3: chart: mychart2 + 4: namespace: ns2 + 5: needs: + 6: - ns1/foo + 7: - name: foo + 8: chart: mychart1 + 9: namespace: ns1 +10: + +merged environment: &{default map[] map[]} +2 release(s) found in helmfile.yaml + Affected releases are: bar (mychart2) UPDATED foo (mychart1) UPDATED diff --git a/pkg/app/diff_test.go b/pkg/app/diff_test.go index 7dc6cfa0..5c015755 100644 --- a/pkg/app/diff_test.go +++ b/pkg/app/diff_test.go @@ -593,7 +593,7 @@ releases: upgraded: []exectest.Release{}, }, { - name: "upgrade when ns2/bar needs ns1/foo", + name: "helm3: upgrade when ns2/bar needs ns1/foo", loc: location(), files: map[string]string{ "/path/to/helmfile.yaml": ` @@ -628,7 +628,7 @@ releases: namespace: ns1 tillerNamespace: tns1 needs: - - tns2/ns2/bar + - tns2/bar - name: bar chart: mychart2 namespace: ns2 @@ -644,7 +644,7 @@ releases: upgraded: []exectest.Release{}, }, { - name: "upgrade when tns2/ns2/bar needs tns1/ns1/foo", + name: "helm2: upgrade when tns2/bar needs tns1/foo", loc: location(), files: map[string]string{ "/path/to/helmfile.yaml": ` @@ -654,7 +654,7 @@ releases: namespace: ns2 tillerNamespace: tns2 needs: - - tns1/ns1/foo + - tns1/foo - name: foo chart: mychart1 namespace: ns1 @@ -681,7 +681,7 @@ first-pass rendering output of "helmfile.yaml.part.0": 4: namespace: ns2 5: tillerNamespace: tns2 6: needs: - 7: - tns1/ns1/foo + 7: - tns1/foo 8: - name: foo 9: chart: mychart1 10: namespace: ns1 @@ -701,7 +701,7 @@ second-pass rendering result of "helmfile.yaml.part.0": 4: namespace: ns2 5: tillerNamespace: tns2 6: needs: - 7: - tns1/ns1/foo + 7: - tns1/foo 8: - name: foo 9: chart: mychart1 10: namespace: ns1 @@ -711,6 +711,74 @@ second-pass rendering result of "helmfile.yaml.part.0": merged environment: &{default map[] map[]} 2 release(s) found in helmfile.yaml +Affected releases are: + bar (mychart2) UPDATED + foo (mychart1) UPDATED + +`, + }, + { + name: "helm3: upgrade when ns2/bar needs ns1/foo", + loc: location(), + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: bar + chart: mychart2 + namespace: ns2 + needs: + - ns1/foo +- name: foo + chart: mychart1 + namespace: ns1 +`, + }, + detailedExitcode: true, + error: "Identified at least one change", + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "bar", Chart: "mychart2", Flags: "--kube-contextdefault--namespacens2--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "foo", Chart: "mychart1", Flags: "--kube-contextdefault--namespacens1--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + upgraded: []exectest.Release{}, + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + log: `processing file "helmfile.yaml" in directory "." +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: bar + 3: chart: mychart2 + 4: namespace: ns2 + 5: needs: + 6: - ns1/foo + 7: - name: foo + 8: chart: mychart1 + 9: namespace: ns1 +10: + +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: bar + 3: chart: mychart2 + 4: namespace: ns2 + 5: needs: + 6: - ns1/foo + 7: - name: foo + 8: chart: mychart1 + 9: namespace: ns1 +10: + +merged environment: &{default map[] map[]} +2 release(s) found in helmfile.yaml + Affected releases are: bar (mychart2) UPDATED foo (mychart1) UPDATED @@ -1263,7 +1331,7 @@ releases: upgraded: []exectest.Release{}, deleted: []exectest.Release{}, concurrency: 1, - error: `in ./helmfile.yaml: release "default/foo" depends on "default/bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, + error: `in ./helmfile.yaml: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, log: `processing file "helmfile.yaml" in directory "." first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= first-pass uses: &{default map[] map[]} @@ -1299,7 +1367,7 @@ second-pass rendering result of "helmfile.yaml.part.0": merged environment: &{default map[] map[]} 2 release(s) found in helmfile.yaml -err: release "default/foo" depends on "default/bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies +err: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies `, }, } diff --git a/pkg/app/snapshot_test.go b/pkg/app/snapshot_test.go new file mode 100644 index 00000000..1ce6bc70 --- /dev/null +++ b/pkg/app/snapshot_test.go @@ -0,0 +1,65 @@ +package app + +import ( + "io/ioutil" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func assertEqualsToSnapshot(t *testing.T, name string, data string) { + type thisPkgLocator struct{} + + t.Helper() + + snapshotFileName := snapshotFileName(t, name) + + if os.Getenv("HELMFILE_UPDATE_SNAPSHOT") != "" { + update(t, snapshotFileName, []byte(data)) + + return + } + + wantData, err := ioutil.ReadFile(snapshotFileName) + if err != nil { + t.Fatalf( + "Snapshot file %q does not exist. Rerun this test with `HELMFILE_UPDATE_SNAPSHOT=1 go test -v -run %s %s` to create the snapshot", + snapshotFileName, + t.Name(), + reflect.TypeOf(thisPkgLocator{}).PkgPath(), + ) + } + + want := string(wantData) + + if d := cmp.Diff(want, data); d != "" { + t.Errorf("unexpected %s: want (-), got (+): %s", name, d) + t.Errorf( + "If you think this is due to the snapshot file being outdated, rerun this test with `HELMFILE_UPDATE_SNAPSHOT=1 go test -v -run %s %s` to update the snapshot", + t.Name(), + reflect.TypeOf(thisPkgLocator{}).PkgPath(), + ) + } +} + +func update(t *testing.T, snapshotFileName string, data []byte) { + t.Helper() + + if err := os.MkdirAll(filepath.Dir(snapshotFileName), 0755); err != nil { + t.Fatalf("%v", err) + } + + if err := ioutil.WriteFile(snapshotFileName, data, 0644); err != nil { + t.Fatalf("%v", err) + } +} + +func snapshotFileName(t *testing.T, name string) string { + dir := filepath.Join(strings.Split(strings.ToLower(t.Name()), "/")...) + + return filepath.Join("testdata", dir, name) +} diff --git a/pkg/app/testdata/testapply_2/deduplicate_by_--selector/log b/pkg/app/testdata/testapply_2/deduplicate_by_--selector/log new file mode 100644 index 00000000..319f8e50 --- /dev/null +++ b/pkg/app/testdata/testapply_2/deduplicate_by_--selector/log @@ -0,0 +1,65 @@ +processing file "helmfile.yaml" in directory "." +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: foo + 3: chart: incubator/raw + 4: namespace: default + 5: labels: + 6: app: test + 7: component: raw + 8: index: '1' + 9: +10: - name: foo +11: chart: incubator/raw +12: namespace: default +13: labels: +14: app: test +15: component: raw +16: index: '2' +17: + +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: foo + 3: chart: incubator/raw + 4: namespace: default + 5: labels: + 6: app: test + 7: component: raw + 8: index: '1' + 9: +10: - name: foo +11: chart: incubator/raw +12: namespace: default +13: labels: +14: app: test +15: component: raw +16: index: '2' +17: + +merged environment: &{default map[] map[]} +1 release(s) matching index=1 found in helmfile.yaml + +Affected releases are: + foo (incubator/raw) UPDATED + +processing 1 groups of releases in this order: +GROUP RELEASES +1 default/default/foo + +processing releases in group 1/1: default/default/foo +getting deployed release version failed:unexpected list key: {^foo$ --kube-contextdefault--deleting--deployed--failed--pending} + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw + diff --git a/pkg/state/state.go b/pkg/state/state.go index 8c6515fb..b311be38 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -338,14 +338,56 @@ func (st *HelmState) ApplyOverrides(spec *ReleaseSpec) { } if st.OverrideNamespace != "" { spec.Namespace = st.OverrideNamespace - - for i := 0; i < len(spec.Needs); i++ { - n := spec.Needs[i] - if len(strings.Split(n, "/")) == 1 { - spec.Needs[i] = st.OverrideNamespace + "/" + n - } - } } + + var needs []string + + // Since the representation differs between needs and id, + // correct it by prepending Namespace and KubeContext. + for i := 0; i < len(spec.Needs); i++ { + n := spec.Needs[i] + + var kubecontext, ns, name string + + components := strings.Split(n, "/") + + name = components[len(components)-1] + + if len(components) > 1 { + ns = components[len(components)-2] + } else if spec.TillerNamespace != "" { + ns = spec.TillerNamespace + } else { + ns = spec.Namespace + } + + if len(components) > 2 { + kubecontext = components[len(components)-3] + } else { + kubecontext = spec.KubeContext + } + + var componentsAfterOverride []string + + if kubecontext != "" { + componentsAfterOverride = append(componentsAfterOverride, kubecontext) + } + + // This is intentionally `kubecontext != "" || ns != ""`, but "ns != "" + // To avoid conflating kubecontext=,namespace=foo,name=bar and kubecontext=foo,namespace=,name=bar + // as they are both `foo/bar`, we explicitly differentiate each with `foo//bar` and `foo/bar`. + // Note that `foo//bar` is not always a equivalent to `foo/default/bar` as the default namespace is depedent on + // the user's kubeconfig. + if kubecontext != "" || ns != "" { + componentsAfterOverride = append(componentsAfterOverride, ns) + } + + componentsAfterOverride = append(componentsAfterOverride, name) + + needs = append(needs, strings.Join(componentsAfterOverride, "/")) + } + + spec.Needs = needs } type RepoUpdater interface { @@ -624,13 +666,25 @@ func ReleaseToID(r *ReleaseSpec) string { } tns := r.TillerNamespace + ns := r.Namespace + if tns != "" { id += tns + "/" + } else if ns != "" { + id += ns + "/" } - ns := r.Namespace - if ns != "" { - id += ns + "/" + if kc != "" { + if tns == "" && ns == "" { + // This is intentional to avoid conflating kc=,ns=foo,name=bar and kc=foo,ns=,name=bar. + // Before https://github.com/roboll/helmfile/pull/1823 they were both `foo/bar` which turned out to break `needs` in many ways. + // + // We now explicitly differentiate each with `foo//bar` and `foo/bar`. + // Note that `foo//bar` is not always a equivalent to `foo/default/bar` as the default namespace is depedent on + // the user's kubeconfig. + // That's why we use `foo//bar` even if it looked unintuitive. + id += "/" + } } id += r.Name diff --git a/pkg/state/state_run.go b/pkg/state/state_run.go index 39a5ac52..e7ea4f4d 100644 --- a/pkg/state/state_run.go +++ b/pkg/state/state_run.go @@ -151,20 +151,20 @@ func GroupReleasesByDependency(releases []Release, opts PlanOptions) ([][]Releas // Only compute dependencies from non-filtered releases if !r.Filtered { - // Since the representation differs between needs and id, - // correct it by prepending KubeContext. var needs []string for i := 0; i < len(r.Needs); i++ { n := r.Needs[i] - if r.KubeContext != "" { - n = r.KubeContext + "/" + n - } needs = append(needs, n) } d.Add(id, dag.Dependencies(needs)) } } + var ids []string + for id := range idToReleases { + ids = append(ids, id) + } + var selectedReleaseIDs []string for _, r := range opts.SelectedReleases { @@ -243,11 +243,11 @@ func GroupReleasesByDependency(releases []Release, opts PlanOptions) ([][]Releas }) for _, id := range idsInGroup { - releases, ok := idToReleases[id] + rs, ok := idToReleases[id] if !ok { - panic(fmt.Errorf("bug: unexpectedly failed to get releases for id %q", id)) + panic(fmt.Errorf("bug: unexpectedly failed to get releases for id %q: %v", id, ids)) } - releasesInGroup = append(releasesInGroup, releases...) + releasesInGroup = append(releasesInGroup, rs...) } result = append(result, releasesInGroup)