Repository: roboll/helmfile
Pull request #1235 (2020-04-29 19:07:53)

Use namespace for release unicity with helm3 (#1213)
This is for #1213 so that the releases are considered scoped to their namespace and not to tiller namespace.

Note the open question here: https://github.com/roboll/helmfile/issues/1213#issuecomment-621402678

mumoshu: @victornoel Thanks! The change LGTM. Regarding the test failure, it's due to that there is now an extra call to helm(helm.IsHelm3) where it wasn't expected before this change.

This is where you can start:

https://github.com/roboll/helmfile/blob/eeb61e61746a5197e2ed722ff23c980a899ea534/pkg/app/app_test.go#L90

In `expectNoCallsToHelm` we currently create `&noCallHelmExec{}`, which panics on any call to helm.

Instead of `noCallHelmExec` which panics on any helm call, you should create a mock that respond to only `IsHelm3`, which is what you added in this PR.

You'd create a new struct for that, which would look like:

```
type versionOnlyHelmExec struct {
   isHelm3 bool
}

func (helm *versionOnlyHelmExec) IsHelm3() bool {
	helm.doPanic()
	return helm.isHelm3
}
```

So that you can use `&versionOnlyHelmExec{isHelm3: false}` instead of `&noCallHelmExec{}` for `TestVisitDesiredStatesWithReleasesFiltered_ReleaseOrder`.

victornoel: @mumoshu I think I've managed to do something correct. I also added some tests :)

Patch:
diff --git a/pkg/app/app.go b/pkg/app/app.go
index 7322799..8cc1815 100644
--- a/pkg/app/app.go
+++ b/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...)
 }
 


Test patch:
diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go
index 86582bc..555b928 100644
--- a/pkg/app/app_test.go
+++ b/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...)
diff --git a/pkg/app/mocks_test.go b/pkg/app/mocks_test.go
index fdd45d4..8d8e7cd 100644
--- a/pkg/app/mocks_test.go
+++ b/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")
 }