Repository: roboll/helmfile
Pull request #576 (2019-05-05 22:31:03)

removes dictionary key for subhelm and uses selectorsInherited
following @mumoshu  comments here https://github.com/roboll/helmfile/pull/567, I removed the usage of subhelmfile path as map key.
I also introduced the *selectorsInherited* key for explicit parent selector inheritance.


Patch:
diff --git a/README.md b/README.md
index 0d98294..6155f88 100644
--- a/README.md
+++ b/README.md
@@ -648,21 +648,21 @@ When composing helmfiles you can use selectors from the command line as well as
 ```yaml
 helmfiles:
 - apps/*/helmfile.yaml
-- apps/a-helmfile.yaml:
+- path: apps/a-helmfile.yaml
     selectors:          # list of selectors
     - name=prometheus      
     - tier=frontend    
-- apps/b-helmfile.yaml: # no selector, so all releases are used
-    selectors: {}
-- apps/c-helmfile.yaml: # parent selector to be used or cli selector for the initial helmfile
-    selectors: inherits
+- path: apps/b-helmfile.yaml # no selector, so all releases are used
+    selectors: []
+- path: apps/c-helmfile.yaml # parent selector to be used or cli selector for the initial helmfile
+    selectorsInherited: true
 ```
 * When a selector is specified, only this selector applies and the parents or CLI selectors are ignored.
 * When not selector is specified there are 2 modes for the selector inheritance because we would like to change the current inheritance behavior (see [issue #344](https://github.com/roboll/helmfile/issues/344)  ).
   * Legacy mode, sub-helmfiles without selectors inherit selectors from their parent helmfile. The initial helmfiles inherit from the command line selectors. 
-  * explicit mode, sub-helmfile without selectors do not inherit from their parent or the CLI selector. If you want them to inherit from their parent selector then use `selectors: inherits`. To enable this explicit mode you need to set the following environment variable `HELMFILE_EXPERIMENTAL=explicit-selector-inheritance` (see [experimental](#experimental-features)).
-* Using `selector: {}` will for all releases to be used regardless of the parent selector or cli for the initial helmfile
-* using `selector: inherits` make the sub-helmfile selects release with the parent selector or the cli for the initial helmfile
+  * explicit mode, sub-helmfile without selectors do not inherit from their parent or the CLI selector. If you want them to inherit from their parent selector then use `selectorsInherited: true`. To enable this explicit mode you need to set the following environment variable `HELMFILE_EXPERIMENTAL=explicit-selector-inheritance` (see [experimental](#experimental-features)).
+* Using `selector: []` will select all releases regardless of the parent selector or cli for the initial helmfile
+* using `selectorsInherited: true` make the sub-helmfile selects releases with the parent selector or the cli for the initial helmfile. You cannot specify an explicit selector while using `selectorsInherited: true`
 
 ## Importing values from any source
 
diff --git a/pkg/app/app.go b/pkg/app/app.go
index d787df1..6d5d351 100644
--- a/pkg/app/app.go
+++ b/pkg/app/app.go
@@ -2,13 +2,15 @@ package app
 
 import (
 	"fmt"
-	"github.com/roboll/helmfile/helmexec"
-	"github.com/roboll/helmfile/state"
-	"go.uber.org/zap"
 	"io/ioutil"
 	"log"
 	"os"
 	"os/signal"
+
+	"github.com/roboll/helmfile/helmexec"
+	"github.com/roboll/helmfile/state"
+	"go.uber.org/zap"
+
 	"path/filepath"
 	"sort"
 	"syscall"
@@ -159,7 +161,7 @@ func (a *App) VisitDesiredStates(fileOrDir string, selector []string, converge f
 			noMatchInSubHelmfiles := true
 			for _, m := range st.Helmfiles {
 				//assign parent selector to sub helm selector in legacy mode or do not inherit in experimental mode
-				if (m.Selectors == nil && !isExplicitSelectorInheritanceEnabled()) || m.Inherits {
+				if (m.Selectors == nil && !isExplicitSelectorInheritanceEnabled()) || m.SelectorsInherited {
 					m.Selectors = selector
 				}
 				if err := a.VisitDesiredStates(m.Path, m.Selectors, converge); err != nil {
diff --git a/state/state.go b/state/state.go
index 2a74763..84cee14 100644
--- a/state/state.go
+++ b/state/state.go
@@ -58,13 +58,11 @@ type HelmState struct {
 
 // SubHelmfileSpec defines the subhelmfile path and options
 type SubHelmfileSpec struct {
-	Path      string   //path or glob pattern for the sub helmfiles
-	Selectors []string //chosen selectors for the sub helmfiles
-	Inherits  bool     //do the sub helmfiles inherits from parent selectors
+	Path               string   //path or glob pattern for the sub helmfiles
+	Selectors          []string //chosen selectors for the sub helmfiles
+	SelectorsInherited bool     //do the sub helmfiles inherits from parent selectors
 }
 
-const InheritsYamlValue = "inherits"
-
 // HelmSpec to defines helmDefault values
 type HelmSpec struct {
 	KubeContext     string   `yaml:"kubeContext"`
@@ -1400,7 +1398,7 @@ func escape(value string) string {
 }
 
 //UnmarshalYAML will unmarshal the helmfile yaml section and fill the SubHelmfileSpec structure
-//this is required to keep allowing string scalar for defining helmfile (maybe)
+//this is required to keep allowing string scalar for defining helmfile
 func (hf *SubHelmfileSpec) UnmarshalYAML(unmarshal func(interface{}) error) error {
 
 	var tmp interface{}
@@ -1409,90 +1407,29 @@ func (hf *SubHelmfileSpec) UnmarshalYAML(unmarshal func(interface{}) error) erro
 	}
 
 	switch i := tmp.(type) {
-	case string: // single path definition without sub items
+	case string: // single path definition without sub items, legacy sub helmfile definition
 		hf.Path = i
 	case map[interface{}]interface{}: // helmfile path with sub section
-		for k, v := range i {
-			switch key := k.(type) {
-			case string:
-				//get the path
-				if key == "path" {
-					hf.Path = v.(string)
-				} else if key == "selectors" {
-					if err := extractSelectorContent(hf, v); err != nil {
-						return err
-					}
-				} else {
-					hf.Path = key
-					//get the selectors if something is specified
-					if v != nil { //we have a path, now compute the selector
-						if err := extractSelector(hf, v); err != nil {
-							return err
-						}
-					} //else it is a path with ending semi-colon without anything else below and it is fine
-				}
-			default:
-				return fmt.Errorf("Expecting a \"string\" scalar for the helmfile collection but got: %v", key)
-			}
+		var subHelmfileSpecTmp struct {
+			Path               string   `yaml:"path"`
+			Selectors          []string `yaml:"selectors"`
+			SelectorsInherited bool     `yaml:"selectorsInherited"`
 		}
+		if err := unmarshal(&subHelmfileSpecTmp); err != nil {
+			return err
+		}
+		hf.Path = subHelmfileSpecTmp.Path
+		hf.Selectors = subHelmfileSpecTmp.Selectors
+		hf.SelectorsInherited = subHelmfileSpecTmp.SelectorsInherited
 	}
 	//since we cannot make sur the "console" string can be red after the "path" we must check we don't have
 	//a SubHelmfileSpec with only selector and no path
 	if hf.Selectors != nil && hf.Path == "" {
 		return fmt.Errorf("found 'selectors' definition without path: %v", hf.Selectors)
 	}
-
-	return nil
-}
-
-//extractSelector this will extract the selectors: from the helmfile section
-//this has been developed to only expect selectors: under the helmfiles for now.
-func extractSelector(hf *SubHelmfileSpec, value interface{}) error {
-	switch value := value.(type) {
-	case map[interface{}]interface{}:
-		for k, v := range value {
-			switch key := k.(type) {
-			case string:
-				if key == "selectors" {
-					return extractSelectorContent(hf, v)
-				} else { //not 'selectors' so error
-					return fmt.Errorf("Expecting a \"selectors\" mapping but got: %v", key)
-				}
-			default: //we where expecting a selector but go something else
-				return fmt.Errorf("Expecting a \"selectors\" mapping for string [-%v] but got: %v of type %T", hf.Path, key, key)
-			}
-		}
-	default:
-		return fmt.Errorf("Expecting a \"selectors\" mapping for string [-%v] but got: %v of type %T", hf.Path, value, value)
-	}
-	return nil
-}
-
-func extractSelectorContent(hf *SubHelmfileSpec, value interface{}) error {
-	switch selectors := value.(type) {
-	case string: //check the string is `inherits` else error
-		if selectors == InheritsYamlValue {
-			hf.Inherits = true
-		} else {
-			return fmt.Errorf("Expecting a list of string, an empty {} or '%v' but got: %v", InheritsYamlValue, selectors)
-		}
-	case []interface{}: //expect an array if strings
-		for _, sel := range selectors {
-			switch selValue := sel.(type) {
-			case string:
-				hf.Selectors = append(hf.Selectors, selValue)
-			default:
-				return fmt.Errorf("Expecting a string, but got: %v", selValue)
-			}
-		}
-	case map[interface{}]interface{}:
-		if len(selectors) == 0 {
-			hf.Selectors = make([]string, 0) //allocate and non nil empty array
-		} else { //unexpected unempty map so error
-			return fmt.Errorf("unexpected unempty map in selector [-%v] but got: %v", hf.Path, selectors)
-		}
-	default:
-		return fmt.Errorf("Expecting list of strings or and empty {} mapping [-%v] but got: [%v] of type [%T] ", hf.Path, selectors, selectors)
+	//also exclude SelectorsInherited to true and explicit selectors
+	if hf.SelectorsInherited && len(hf.Selectors) > 0 {
+		return fmt.Errorf("You cannot use 'SelectorsInherited: true' along with and explicit selector for path: %v", hf.Path)
 	}
 	return nil
 }


Test patch:
diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go
index 713df53..99fe80b 100644
--- a/pkg/app/app_test.go
+++ b/pkg/app/app_test.go
@@ -2,15 +2,16 @@ package app
 
 import (
 	"fmt"
-	"github.com/roboll/helmfile/helmexec"
-	"github.com/roboll/helmfile/state"
-	"gotest.tools/env"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
+
+	"github.com/roboll/helmfile/helmexec"
+	"github.com/roboll/helmfile/state"
+	"gotest.tools/env"
 )
 
 type testFs struct {
@@ -329,13 +330,13 @@ func TestVisitDesiredStatesWithReleasesFiltered_EmbeddedSelectors(t *testing.T)
 	files := map[string]string{
 		"/path/to/helmfile.yaml": `
 helmfiles:
-- helmfile.d/a*.yaml:
-    selectors:
-    - name=prometheus      
-    - name=zipkin      
+- path: helmfile.d/a*.yaml
+  selectors:
+  - name=prometheus      
+  - name=zipkin      
 - helmfile.d/b*.yaml
-- helmfile.d/c*.yaml:
-    selectors: {}
+- path: helmfile.d/c*.yaml
+  selectors: []
 `,
 		"/path/to/helmfile.d/a1.yaml": `
 releases:
@@ -462,17 +463,17 @@ func TestVisitDesiredStatesWithReleasesFiltered_InheritedSelectors_inherits(t *t
 		"/path/to/helmfile.yaml": `
 helmfiles:
 - helmfile.d/a*.yaml
-- helmfile.d/a*.yaml:
-    selectors:
-    - select=foo
+- path: helmfile.d/a*.yaml
+  selectors:
+  - select=foo
 releases:
 - name: mongodb
   chart: stable/mongodb
 `,
 		"/path/to/helmfile.d/a.yaml": `
 helmfiles:
-- b*.yaml:
-    selectors: inherits
+- path: b*.yaml
+  selectorsInherited: true
 releases:
 - name: zipkin
   chart: stable/zipkin
diff --git a/state/create_test.go b/state/create_test.go
index d66efed..d99d9b6 100644
--- a/state/create_test.go
+++ b/state/create_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	. "gotest.tools/assert"
+	"gotest.tools/assert/cmp"
 )
 
 func TestReadFromYaml(t *testing.T) {
@@ -261,45 +262,26 @@ func TestReadFromYaml_Helmfiles_Selectors(t *testing.T) {
 			path: "working/selector",
 			content: []byte(`helmfiles:
 - simple/helmfile.yaml
-- simple/helmfile/with/semicolon.yaml:
-- two/selectors.yaml:
-    selectors:
-      - name=foo
-      - name=bar
-- empty/selector.yaml:
-    selectors: {}
-- inherits/selector.yaml:
-    selectors: inherits
 - path: path/prefix/selector.yaml
   selectors:
     - name=zorba
+    - foo=bar
 - path: path/prefix/empty/selector.yaml
-  selectors: {}
+  selectors: []
 - path: path/prefix/inherits/selector.yaml
-  selectors: inherits
+  selectorsInherited: true
 `),
 			wantErr: false,
-			helmfiles: []SubHelmfileSpec{{Path: "simple/helmfile.yaml"},
-				{Path: "simple/helmfile/with/semicolon.yaml", Selectors: nil, Inherits: false},
-				{Path: "two/selectors.yaml", Selectors: []string{"name=foo", "name=bar"}, Inherits: false},
-				{Path: "empty/selector.yaml", Selectors: []string{}, Inherits: false},
-				{Path: "inherits/selector.yaml", Selectors: nil, Inherits: true},
-				{Path: "path/prefix/selector.yaml", Selectors: []string{"name=zorba"}, Inherits: false},
-				{Path: "path/prefix/empty/selector.yaml", Selectors: []string{}, Inherits: false},
-				{Path: "path/prefix/inherits/selector.yaml", Selectors: nil, Inherits: true},
+			helmfiles: []SubHelmfileSpec{{Path: "simple/helmfile.yaml", Selectors: nil, SelectorsInherited: false},
+				{Path: "path/prefix/selector.yaml", Selectors: []string{"name=zorba", "foo=bar"}, SelectorsInherited: false},
+				{Path: "path/prefix/empty/selector.yaml", Selectors: []string{}, SelectorsInherited: false},
+				{Path: "path/prefix/inherits/selector.yaml", Selectors: nil, SelectorsInherited: true},
 			},
 		},
-		{
-			path: "failing1/selector",
-			content: []byte(`helmfiles:
-- failing1/helmfile.yaml: foo
-`),
-			wantErr: true,
-		},
 		{
 			path: "failing2/selector",
 			content: []byte(`helmfiles:
-- failing2/helmfile.yaml: 
+- path: failing2/helmfile.yaml
     wrongkey:
 `),
 			wantErr: true,
@@ -307,7 +289,7 @@ func TestReadFromYaml_Helmfiles_Selectors(t *testing.T) {
 		{
 			path: "failing3/selector",
 			content: []byte(`helmfiles:
-- failing3/helmfile.yaml: 
+- path: failing3/helmfile.yaml
     selectors: foo
 `),
 			wantErr: true,
@@ -315,7 +297,7 @@ func TestReadFromYaml_Helmfiles_Selectors(t *testing.T) {
 		{
 			path: "failing4/selector",
 			content: []byte(`helmfiles:
-- failing4/helmfile.yaml: 
+- path: failing4/helmfile.yaml
     selectors:
 `),
 			wantErr: true,
@@ -323,25 +305,27 @@ func TestReadFromYaml_Helmfiles_Selectors(t *testing.T) {
 		{
 			path: "failing4/selector",
 			content: []byte(`helmfiles:
-- failing4/helmfile.yaml: 
+- path: failing4/helmfile.yaml
     selectors:
       - colon: not-authorized
 `),
 			wantErr: true,
 		},
 		{
-			path: "failing5/selector",
+			path: "failing6/selector",
 			content: []byte(`helmfiles:
 - selectors:
-    - colon: not-authorized
+    - whatever
 `),
 			wantErr: true,
 		},
 		{
-			path: "failing6/selector",
+			path: "failing7/selector",
 			content: []byte(`helmfiles:
-- selectors:
-    - whatever
+- path: foo/bar
+  selectors:
+  - foo=bar
+  selectorsInherited: true
 `),
 			wantErr: true,
 		},
@@ -355,6 +339,6 @@ func TestReadFromYaml_Helmfiles_Selectors(t *testing.T) {
 				t.Error("unexpected error:", err)
 			}
 		}
-		DeepEqual(t, st.Helmfiles, test.helmfiles)
+		Assert(t, cmp.DeepEqual(st.Helmfiles, test.helmfiles), "for path %v", test.path)
 	}
 }