From c731227e9a674752b6a0a1e29ac6d26d95c92eb7 Mon Sep 17 00:00:00 2001 From: Tunahan Sezen Date: Fri, 8 Dec 2023 16:42:40 +0300 Subject: [PATCH] fix: --state-values-set unable to set booleans (#1199) This pr fixes auto-wrapping of booleans and integers into quotes when using --state-values-set by: - Adding: --state-values-set-string flag for intentional string set of boolean or integer - Changing: --state-values-set flag not wrapping now - Removing - Resolves https://github.com/roboll/helmfile/issues/1347 Signed-off-by: Tunahan Sezen --- cmd/root.go | 1 + pkg/config/config.go | 19 ++++++++- pkg/config/global.go | 10 ++++- pkg/maputil/maputil.go | 65 +++++++++++++++++++++++++++--- pkg/maputil/maputil_test.go | 80 +++++++++++++++++++++++++++++++++++-- 5 files changed, 164 insertions(+), 11 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 9038987f..9192d8c0 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -119,6 +119,7 @@ func setGlobalOptionsForRootCmd(fs *pflag.FlagSet, globalOptions *config.GlobalO fs.StringVarP(&globalOptions.File, "file", "f", "", "load config from file or directory. defaults to \"`helmfile.yaml`\" or \"helmfile.yaml.gotmpl\" or \"helmfile.d\" (means \"helmfile.d/*.yaml\" or \"helmfile.d/*.yaml.gotmpl\") in this preference. Specify - to load the config from the standard input.") fs.StringVarP(&globalOptions.Environment, "environment", "e", "", `specify the environment name. Overrides "HELMFILE_ENVIRONMENT" OS environment variable when specified. defaults to "default"`) fs.StringArrayVar(&globalOptions.StateValuesSet, "state-values-set", nil, "set state values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2). Used to override .Values within the helmfile template (not values template).") + fs.StringArrayVar(&globalOptions.StateValuesSetString, "state-values-set-string", nil, "set state STRING values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2). Used to override .Values within the helmfile template (not values template).") fs.StringArrayVar(&globalOptions.StateValuesFile, "state-values-file", nil, "specify state values in a YAML file. Used to override .Values within the helmfile template (not values template).") fs.BoolVar(&globalOptions.SkipDeps, "skip-deps", false, `skip running "helm repo update" and "helm dependency build"`) fs.BoolVar(&globalOptions.StripArgsValuesOnExitError, "strip-args-values-on-exit-error", true, `Strip the potential secret values of the helm command args contained in a helmfile error message`) diff --git a/pkg/config/config.go b/pkg/config/config.go index 239e00d2..7bfe1aca 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -7,7 +7,7 @@ import ( ) func NewCLIConfigImpl(g *GlobalImpl) error { - optsSet := g.RawStateValuesSet() + optsSet := g.RawStateValuesSetString() if len(optsSet) > 0 { set := map[string]any{} for i := range optsSet { @@ -17,7 +17,22 @@ func NewCLIConfigImpl(g *GlobalImpl) error { k := maputil.ParseKey(op[0]) v := op[1] - maputil.Set(set, k, v) + maputil.Set(set, k, v, true) + } + } + g.SetSet(set) + } + optsSet = g.RawStateValuesSet() + if len(optsSet) > 0 { + set := map[string]any{} + for i := range optsSet { + ops := strings.Split(optsSet[i], ",") + for j := range ops { + op := strings.SplitN(ops[j], "=", 2) + k := maputil.ParseKey(op[0]) + v := op[1] + + maputil.Set(set, k, v, false) } } g.SetSet(set) diff --git a/pkg/config/global.go b/pkg/config/global.go index 0132fe1a..0580343b 100644 --- a/pkg/config/global.go +++ b/pkg/config/global.go @@ -8,6 +8,7 @@ import ( "go.uber.org/zap" "golang.org/x/term" + "github.com/helmfile/helmfile/pkg/maputil" "github.com/helmfile/helmfile/pkg/state" ) @@ -23,6 +24,8 @@ type GlobalOptions struct { Environment string // StateValuesSet is a list of state values to set on the command line. StateValuesSet []string + // StateValuesSetString is a list of state values to set on the command line. + StateValuesSetString []string // StateValuesFiles is a list of state values files to use. StateValuesFile []string // SkipDeps is true if the running "helm repo update" and "helm dependency build" should be skipped @@ -87,7 +90,7 @@ func NewGlobalImpl(opts *GlobalOptions) *GlobalImpl { // Setset sets the set func (g *GlobalImpl) SetSet(set map[string]any) { - g.set = set + g.set = maputil.MergeMaps(g.set, set) } // HelmBinary returns the path to the Helm binary. @@ -135,6 +138,11 @@ func (g *GlobalImpl) RawStateValuesSet() []string { return g.GlobalOptions.StateValuesSet } +// RawStateValuesSetString returns the set +func (g *GlobalImpl) RawStateValuesSetString() []string { + return g.GlobalOptions.StateValuesSetString +} + // StateValuesFiles returns the state values files func (g *GlobalImpl) StateValuesFiles() []string { return g.GlobalOptions.StateValuesFile diff --git a/pkg/maputil/maputil.go b/pkg/maputil/maputil.go index 105e0957..8df924ee 100644 --- a/pkg/maputil/maputil.go +++ b/pkg/maputil/maputil.go @@ -2,6 +2,7 @@ package maputil import ( "fmt" + "strconv" "strings" ) @@ -65,7 +66,7 @@ func recursivelyStringifyMapKey(v any) (any, error) { type arg interface { getMap(map[string]any) map[string]any - set(map[string]any, string) + set(map[string]any, any) } type keyArg struct { @@ -85,7 +86,7 @@ func (a keyArg) getMap(m map[string]any) map[string]any { } } -func (a keyArg) set(m map[string]any, value string) { +func (a keyArg) set(m map[string]any, value any) { m[a.key] = value } @@ -125,7 +126,7 @@ func (a indexedKeyArg) getMap(m map[string]any) map[string]any { } } -func (a indexedKeyArg) set(m map[string]any, value string) { +func (a indexedKeyArg) set(m map[string]any, value any) { t := a.getArray(m) t[a.index] = value m[a.key] = t @@ -178,7 +179,7 @@ func ParseKey(key string) []string { return r } -func Set(m map[string]any, key []string, value string) { +func Set(m map[string]any, key []string, value string, stringBool bool) { if len(key) == 0 { panic(fmt.Errorf("bug: unexpected length of key: %d", len(key))) } @@ -187,5 +188,59 @@ func Set(m map[string]any, key []string, value string) { m, key = getCursor(key[0]).getMap(m), key[1:] } - getCursor(key[0]).set(m, value) + getCursor(key[0]).set(m, typedVal(value, stringBool)) +} + +func typedVal(val string, st bool) any { + // if st is true, directly return it without casting it + if st { + return val + } + + if strings.EqualFold(val, "true") { + return true + } + + if strings.EqualFold(val, "false") { + return false + } + + if strings.EqualFold(val, "null") { + return nil + } + + // handling of only zero, if val has zero prefix, it will be considered as string + if strings.EqualFold(val, "0") { + return int64(0) + } + + // If this value does not start with zero, try parsing it to an int + if len(val) != 0 && val[0] != '0' { + if iv, err := strconv.ParseInt(val, 10, 64); err == nil { + return iv + } + } + + return val +} + +func MergeMaps(a, b map[string]interface{}) map[string]interface{} { + out := make(map[string]interface{}, len(a)) + // fill the out map with the first map + for k, v := range a { + out[k] = v + } + for k, v := range b { + if v, ok := v.(map[string]interface{}); ok { + if bv, ok := out[k]; ok { + if bv, ok := bv.(map[string]interface{}); ok { + // if b and out map has a map value, merge it too + out[k] = MergeMaps(bv, v) + continue + } + } + } + out[k] = v + } + return out } diff --git a/pkg/maputil/maputil_test.go b/pkg/maputil/maputil_test.go index be60d756..837c5b4e 100644 --- a/pkg/maputil/maputil_test.go +++ b/pkg/maputil/maputil_test.go @@ -69,7 +69,7 @@ func TestMapUtil_KeyArg(t *testing.T) { key := []string{"a", "b", "c"} - Set(m, key, "C") + Set(m, key, "C", false) c := (((m["a"].(map[string]any))["b"]).(map[string]any))["c"] @@ -83,7 +83,7 @@ func TestMapUtil_IndexedKeyArg(t *testing.T) { key := []string{"a", "b[0]", "c"} - Set(m, key, "C") + Set(m, key, "C", false) c := (((m["a"].(map[string]any))["b"].([]any))[0].(map[string]any))["c"] @@ -124,7 +124,7 @@ func TestMapUtil_IndexedKeyArg2(t *testing.T) { k := ParseKey(op[0]) v := op[1] - Set(set, k, v) + Set(set, k, v, false) } } if !reflect.DeepEqual(set, c.want) { @@ -174,3 +174,77 @@ func TestMapUtil_ParseKey(t *testing.T) { } } } + +func TestMapUtil_typedVal(t *testing.T) { + typedValueTest(t, "true", true) + typedValueTest(t, "null", nil) + typedValueTest(t, "0", int64(0)) + typedValueTest(t, "5", int64(5)) + typedValueTest(t, "05", "05") +} + +func typedValueTest(t *testing.T, input string, expectedWhenNoStr any) { + returnValue := typedVal(input, true) + if returnValue != input { + t.Errorf("unexpected typed value: expected=%s, got=%s", input, returnValue) + } + returnValue = typedVal(input, false) + if returnValue != expectedWhenNoStr { + t.Errorf("unexpected typed value: expected=%s, got=%s", input, returnValue) + } +} + +func TestMapUtil_MergeMaps(t *testing.T) { + map1 := map[string]interface{}{ + "debug": true, + } + map2 := map[string]interface{}{ + "logLevel": "info", + "replicaCount": 3, + } + map3 := map[string]interface{}{ + "logLevel": "info", + "replicaCount": map[string]any{ + "app1": 3, + "awesome": 4, + }, + } + map4 := map[string]interface{}{ + "logLevel": "info", + "replicaCount": map[string]any{ + "app1": 3, + }, + } + + testMap := MergeMaps(map2, map4) + equal := reflect.DeepEqual(testMap, map4) + if !equal { + t.Errorf("Expected a nested map to overwrite a flat value. Expected: %v, got %v", map4, testMap) + } + + testMap = MergeMaps(map4, map2) + equal = reflect.DeepEqual(testMap, map2) + if !equal { + t.Errorf("Expected a flat value to overwrite a map. Expected: %v, got %v", map2, testMap) + } + + testMap = MergeMaps(map4, map3) + equal = reflect.DeepEqual(testMap, map3) + if !equal { + t.Errorf("Expected a nested map to overwrite another nested map. Expected: %v, got %v", map3, testMap) + } + + testMap = MergeMaps(map1, map3) + expectedMap := map[string]interface{}{ + "debug": true, + "logLevel": "info", + "replicaCount": map[string]any{ + "app1": 3, + "awesome": 4, + }, + } + equal = reflect.DeepEqual(testMap, expectedMap) + if !equal { + t.Errorf("Expected a map with different keys to merge properly with another map. Expected: %v, got %v", expectedMap, testMap) + } +}