Use namespace for release unicity with helm3 (#1213) (#1235)

This is for #1213 so that the releases are considered scoped to their namespace and not to tiller namespace.

Fixes #1213

pkg/app/app.go

@@ -699,7 +699,7 @@ func (a *App) visitStatesWithSelectorsAndRemoteSupport(fileOrDir string, converg
 	return a.visitStates(fileOrDir, opts, converge)
 }
 
-func processFilteredReleases(st *state.HelmState, converge func(st *state.HelmState) []error) (bool, []error) {
+func processFilteredReleases(st *state.HelmState, helm helmexec.Interface, converge func(st *state.HelmState) []error) (bool, []error) {
 	if len(st.Selectors) > 0 {
 		err := st.FilterReleases()
 		if err != nil {
@@ -713,11 +713,15 @@ func processFilteredReleases(st *state.HelmState, converge func(st *state.HelmSt
 
 	releaseNameCounts := map[Key]int{}
 	for _, r := range st.Releases {
-		tillerNamespace := st.HelmDefaults.TillerNamespace
-		if r.TillerNamespace != "" {
-			tillerNamespace = r.TillerNamespace
+		namespace := r.Namespace
+		if !helm.IsHelm3() {
+			if r.TillerNamespace != "" {
+				namespace = r.TillerNamespace
+			} else {
+				namespace = st.HelmDefaults.TillerNamespace
+			}
 		}
-		releaseNameCounts[Key{tillerNamespace, r.Name}]++
+		releaseNameCounts[Key{namespace, r.Name}]++
 	}
 	for name, c := range releaseNameCounts {
 		if c > 1 {
@@ -734,7 +738,7 @@ func processFilteredReleases(st *state.HelmState, converge func(st *state.HelmSt
 
 func (a *App) Wrap(converge func(*state.HelmState, helmexec.Interface) []error) func(st *state.HelmState, helm helmexec.Interface) (bool, []error) {
 	return func(st *state.HelmState, helm helmexec.Interface) (bool, []error) {
-		return processFilteredReleases(st, func(st *state.HelmState) []error {
+		return processFilteredReleases(st, helm, func(st *state.HelmState) []error {
 			return converge(st, helm)
 		})
 	}
@@ -742,7 +746,7 @@ func (a *App) Wrap(converge func(*state.HelmState, helmexec.Interface) []error)
 
 func (a *App) VisitDesiredStatesWithReleasesFiltered(fileOrDir string, converge func(*state.HelmState) []error, o ...LoadOption) error {
 	return a.visitStatesWithSelectorsAndRemoteSupport(fileOrDir, func(st *state.HelmState) (bool, []error) {
-		return processFilteredReleases(st, converge)
+		return processFilteredReleases(st, a.getHelm(st), converge)
 	}, o...)
 }
 

pkg/app/app_test.go

@@ -45,12 +45,16 @@ func injectFs(app *App, fs *testhelper.TestFs) *App {
 }
 
 func expectNoCallsToHelm(app *App) {
+	expectNoCallsToHelmVersion(app, false)
+}
+
+func expectNoCallsToHelmVersion(app *App, isHelm3 bool) {
 	if app.helms != nil {
 		panic("invalid call to expectNoCallsToHelm")
 	}
 
 	app.helms = map[helmKey]helmexec.Interface{
-		createHelmKey(app.OverrideHelmBinary, app.OverrideKubeContext): &noCallHelmExec{},
+		createHelmKey(app.OverrideHelmBinary, app.OverrideKubeContext): &versionOnlyHelmExec{isHelm3: isHelm3},
 	}
 }
 
@@ -1243,6 +1247,178 @@ releases:
 	}
 }
 
+// See https://github.com/roboll/helmfile/issues/1213
+func TestVisitDesiredStatesWithReleases_DuplicateReleasesHelm2(t *testing.T) {
+	files := map[string]string{
+		"/path/to/helmfile.yaml": `
+releases:
+- name: foo
+  namespace: foo
+  chart: charts/foo
+- name: foo
+  namespace: bar
+  chart: charts/foo
+`,
+	}
+
+	actual := []state.ReleaseSpec{}
+
+	collectReleases := func(st *state.HelmState) []error {
+		for _, r := range st.Releases {
+			actual = append(actual, r)
+		}
+		return []error{}
+	}
+	app := appWithFs(&App{
+		OverrideHelmBinary:  DefaultHelmBinary,
+		OverrideKubeContext: "default",
+		Logger:              helmexec.NewLogger(os.Stderr, "debug"),
+		Namespace:           "",
+		Env:                 "default",
+	}, files)
+
+	expectNoCallsToHelmVersion(app, false)
+
+	err := app.VisitDesiredStatesWithReleasesFiltered(
+		"helmfile.yaml", collectReleases,
+	)
+
+	expected := "in ./helmfile.yaml: duplicate release \"foo\" found in \"\": there were 2 releases named \"foo\" matching specified selector"
+	if err == nil {
+		t.Errorf("error expected but not happened")
+	} else if err.Error() != expected {
+		t.Errorf("unexpected error message: expected=\"%s\", actual=\"%s\"", expected, err.Error())
+	}
+}
+
+// See https://github.com/roboll/helmfile/issues/1213
+func TestVisitDesiredStatesWithReleases_NoDuplicateReleasesHelm2(t *testing.T) {
+	files := map[string]string{
+		"/path/to/helmfile.yaml": `
+releases:
+- name: foo
+  namespace: foo
+  tillerNamespace: tns1
+  chart: charts/foo
+- name: foo
+  namespace: bar
+  tillerNamespace: tns2
+  chart: charts/foo
+`,
+	}
+
+	actual := []state.ReleaseSpec{}
+
+	collectReleases := func(st *state.HelmState) []error {
+		for _, r := range st.Releases {
+			actual = append(actual, r)
+		}
+		return []error{}
+	}
+	app := appWithFs(&App{
+		OverrideHelmBinary:  DefaultHelmBinary,
+		OverrideKubeContext: "default",
+		Logger:              helmexec.NewLogger(os.Stderr, "debug"),
+		Namespace:           "",
+		Env:                 "default",
+	}, files)
+
+	expectNoCallsToHelmVersion(app, false)
+
+	err := app.VisitDesiredStatesWithReleasesFiltered(
+		"helmfile.yaml", collectReleases,
+	)
+
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+// See https://github.com/roboll/helmfile/issues/1213
+func TestVisitDesiredStatesWithReleases_NoDuplicateReleasesHelm3(t *testing.T) {
+	files := map[string]string{
+		"/path/to/helmfile.yaml": `
+releases:
+- name: foo
+  namespace: foo
+  chart: charts/foo
+- name: foo
+  namespace: bar
+  chart: charts/foo
+`,
+	}
+
+	actual := []state.ReleaseSpec{}
+
+	collectReleases := func(st *state.HelmState) []error {
+		for _, r := range st.Releases {
+			actual = append(actual, r)
+		}
+		return []error{}
+	}
+	app := appWithFs(&App{
+		OverrideHelmBinary:  DefaultHelmBinary,
+		OverrideKubeContext: "default",
+		Logger:              helmexec.NewLogger(os.Stderr, "debug"),
+		Namespace:           "",
+		Env:                 "default",
+	}, files)
+
+	expectNoCallsToHelmVersion(app, true)
+
+	err := app.VisitDesiredStatesWithReleasesFiltered(
+		"helmfile.yaml", collectReleases,
+	)
+
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+// See https://github.com/roboll/helmfile/issues/1213
+func TestVisitDesiredStatesWithReleases_DuplicateReleasesHelm3(t *testing.T) {
+	files := map[string]string{
+		"/path/to/helmfile.yaml": `
+releases:
+- name: foo
+  namespace: foo
+  chart: charts/foo
+- name: foo
+  namespace: foo
+  chart: charts/foo
+`,
+	}
+
+	actual := []state.ReleaseSpec{}
+
+	collectReleases := func(st *state.HelmState) []error {
+		for _, r := range st.Releases {
+			actual = append(actual, r)
+		}
+		return []error{}
+	}
+	app := appWithFs(&App{
+		OverrideHelmBinary:  DefaultHelmBinary,
+		OverrideKubeContext: "default",
+		Logger:              helmexec.NewLogger(os.Stderr, "debug"),
+		Namespace:           "",
+		Env:                 "default",
+	}, files)
+
+	expectNoCallsToHelmVersion(app, true)
+
+	err := app.VisitDesiredStatesWithReleasesFiltered(
+		"helmfile.yaml", collectReleases,
+	)
+
+	expected := "in ./helmfile.yaml: duplicate release \"foo\" found in \"foo\": there were 2 releases named \"foo\" matching specified selector"
+	if err == nil {
+		t.Errorf("error expected but not happened")
+	} else if err.Error() != expected {
+		t.Errorf("unexpected error message: expected=\"%s\", actual=\"%s\"", expected, err.Error())
+	}
+}
+
 func TestLoadDesiredStateFromYaml_DuplicateReleaseName(t *testing.T) {
 	yamlFile := "example/path/to/yaml/file"
 	yamlContent := []byte(`releases:
@@ -3751,6 +3927,8 @@ releases:
 		Namespace:           "testNamespace",
 	}, files)
 
+	expectNoCallsToHelm(app)
+
 	out := captureStdout(func() {
 		err := app.PrintState(configImpl{})
 		assert.NilError(t, err)
@@ -3795,6 +3973,9 @@ releases:
 		Logger:              logger,
 		Namespace:           "testNamespace",
 	}, files)
+
+	expectNoCallsToHelm(app)
+
 	out := captureStdout(func() {
 		err := app.PrintState(configImpl{})
 		assert.NilError(t, err)
@@ -3951,6 +4132,8 @@ releases:
 		Env:                 "default",
 	}, files)
 
+	expectNoCallsToHelm(app)
+
 	var specs []state.ReleaseSpec
 	collectReleases := func(st *state.HelmState) []error {
 		specs = append(specs, st.Releases...)

pkg/app/mocks_test.go

@@ -5,6 +5,15 @@ import "github.com/roboll/helmfile/pkg/helmexec"
 type noCallHelmExec struct {
 }
 
+type versionOnlyHelmExec struct {
+	*noCallHelmExec
+	isHelm3 bool
+}
+
+func (helm *versionOnlyHelmExec) IsHelm3() bool {
+	return helm.isHelm3
+}
+
 func (helm *noCallHelmExec) doPanic() {
 	panic("unexpected call to helm")
 }