Add MergeMapsWithArrayMerge and IsCLIOverride flag - WIP fixing array merging issue

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

pkg/app/app.go

@@ -1268,8 +1268,10 @@ func (a *App) visitStatesWithSelectorsAndRemoteSupportWithContext(fileOrDir stri
 		envvals = append(envvals, v)
 	}
 
+	// Append CLI set to envvals for loading, and also pass it separately to mark it as CLI override
 	if len(a.Set) > 0 {
 		envvals = append(envvals, a.Set)
+		opts.Environment.CLISet = a.Set
 	}
 
 	if len(envvals) > 0 {

pkg/app/desired_state_file_loader.go

@@ -64,9 +64,14 @@ func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, e
 			return nil, err
 		}
 
+		// CLI overrides (--state-values-set) should use element-by-element array merging
+		// CLISet is set when there are CLI overrides
+		isCLIOverride := len(opts.Environment.CLISet) > 0
+
 		overrodeEnv = &environment.Environment{
-			Name:   ld.env,
-			Values: vals,
+			Name:          ld.env,
+			Values:        vals,
+			IsCLIOverride: isCLIOverride,
 		}
 	}
 

pkg/config/global.go

@@ -102,7 +102,7 @@ func NewGlobalImpl(opts *GlobalOptions) *GlobalImpl {
 
 // Setset sets the set
 func (g *GlobalImpl) SetSet(set map[string]any) {
-	g.set = maputil.MergeMaps(g.set, set)
+	g.set = maputil.MergeMapsWithArrayMerge(g.set, set)
 }
 
 // HelmBinary returns the path to the Helm binary.

pkg/environment/environment.go

@@ -12,6 +12,9 @@ type Environment struct {
 	KubeContext string
 	Values      map[string]any
 	Defaults    map[string]any
+	// IsCLIOverride indicates if this environment contains CLI overrides from --state-values-set
+	// When true, arrays are merged element-by-element. When false, arrays are replaced.
+	IsCLIOverride bool
 }
 
 var EmptyEnvironment Environment
@@ -54,10 +57,11 @@ func (e Environment) DeepCopy() Environment {
 	}
 
 	return Environment{
-		Name:        e.Name,
-		KubeContext: e.KubeContext,
-		Values:      values,
-		Defaults:    defaults,
+		Name:          e.Name,
+		KubeContext:   e.KubeContext,
+		Values:        values,
+		Defaults:      defaults,
+		IsCLIOverride: e.IsCLIOverride,
 	}
 }
 
@@ -74,6 +78,10 @@ func (e *Environment) Merge(other *Environment) (*Environment, error) {
 		if err := mergo.Merge(&copy, other, mergo.WithOverride); err != nil {
 			return nil, err
 		}
+		// Preserve the IsCLIOverride flag from other
+		if other.IsCLIOverride {
+			copy.IsCLIOverride = true
+		}
 	}
 	return &copy, nil
 }
@@ -81,10 +89,15 @@ func (e *Environment) Merge(other *Environment) (*Environment, error) {
 func (e *Environment) GetMergedValues() (map[string]any, error) {
 	vals := map[string]any{}
 
-	// Use MergeMaps instead of mergo.Merge to properly handle array merging element-by-element
-	// This fixes issue #2281 where arrays were being replaced entirely instead of merged
-	vals = maputil.MergeMaps(vals, e.Defaults)
-	vals = maputil.MergeMaps(vals, e.Values)
+	// For CLI overrides (--state-values-set), use MergeMapsWithArrayMerge to handle array indices
+	// For helmfile composition, use regular MergeMaps which replaces arrays (documented behavior)
+	if e.IsCLIOverride {
+		vals = maputil.MergeMapsWithArrayMerge(vals, e.Defaults)
+		vals = maputil.MergeMapsWithArrayMerge(vals, e.Values)
+	} else {
+		vals = maputil.MergeMaps(vals, e.Defaults)
+		vals = maputil.MergeMaps(vals, e.Values)
+	}
 
 	vals, err := maputil.CastKeysToStrings(vals)
 	if err != nil {

pkg/maputil/maputil.go

@@ -243,6 +243,35 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} {
 				}
 			}
 		}
+		// Arrays are replaced, not merged (documented behavior)
+		out[k] = v
+	}
+	return out
+}
+
+// MergeMapsWithArrayMerge merges two maps, merging arrays element-by-element.
+// This is used specifically for CLI overrides (--state-values-set) where sparse arrays
+// with specific indices set should merge with base arrays element-by-element.
+// For general helmfile composition, use MergeMaps which replaces arrays.
+func MergeMapsWithArrayMerge(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 == nil {
+			continue
+		}
+		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] = MergeMapsWithArrayMerge(bv, v)
+					continue
+				}
+			}
+		}
 		// Handle array merging element-by-element
 		vSlice := toInterfaceSlice(v)
 		if vSlice != nil {
@@ -292,11 +321,11 @@ func mergeSlices(base, override []any) []any {
 			continue
 		}
 
-		// If both are maps, merge them recursively
+		// If both are maps, merge them recursively with array merging
 		if overrideMap, ok := overrideVal.(map[string]any); ok {
 			if i < len(base) {
 				if baseMap, ok := base[i].(map[string]any); ok {
-					result[i] = MergeMaps(baseMap, overrideMap)
+					result[i] = MergeMapsWithArrayMerge(baseMap, overrideMap)
 					continue
 				}
 			}

pkg/maputil/maputil_test.go

@@ -474,7 +474,9 @@ func TestMapUtil_Issue2281_EmptyMapScenario(t *testing.T) {
 	})
 }
 
-// TestMapUtil_Issue2281_MergeArrays tests that MergeMaps should merge arrays element-by-element
+// TestMapUtil_Issue2281_MergeArrays tests that MergeMapsWithArrayMerge should merge arrays element-by-element
+// This is used for CLI overrides (--state-values-set) where sparse arrays should merge with base arrays.
+// For general helmfile composition, MergeMaps replaces arrays entirely (documented behavior).
 func TestMapUtil_Issue2281_MergeArrays(t *testing.T) {
 	t.Run("merging arrays should preserve elements from base that aren't in override", func(t *testing.T) {
 		// Base values from helmfile
@@ -491,7 +493,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) {
 			},
 		}
 
-		result := MergeMaps(base, override)
+		result := MergeMapsWithArrayMerge(base, override)
 
 		// Expected: array should be ["cmdlinething1", "thing2"]
 		// array[0] is overridden, array[1] is preserved from base
@@ -532,7 +534,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) {
 			},
 		}
 
-		result := MergeMaps(base, override)
+		result := MergeMapsWithArrayMerge(base, override)
 
 		// Expected: complexArray[0] should be unchanged, complexArray[1] should have merged fields
 		resultArray := result["top"].(map[string]any)["complexArray"].([]any)
@@ -558,7 +560,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) {
 		}
 	})
 
-	t.Run("complete issue #2281 scenario with MergeMaps", func(t *testing.T) {
+	t.Run("complete issue #2281 scenario with MergeMapsWithArrayMerge", func(t *testing.T) {
 		// Base values from helmfile
 		base := map[string]interface{}{
 			"top": map[string]any{
@@ -591,7 +593,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) {
 			},
 		}
 
-		result := MergeMaps(base, override)
+		result := MergeMapsWithArrayMerge(base, override)
 
 		// Check array
 		resultArray := result["top"].(map[string]any)["array"].([]any)
@@ -617,3 +619,90 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) {
 		}
 	})
 }
+
+// TestMapUtil_MergeMaps_ArrayReplacement tests that MergeMaps replaces arrays
+// This is the documented behavior for helmfile composition and layer merging
+func TestMapUtil_MergeMaps_ArrayReplacement(t *testing.T) {
+	t.Run("arrays should be replaced not merged", func(t *testing.T) {
+		base := map[string]interface{}{
+			"list": []any{
+				map[string]any{"name": "dummy", "values": []any{1, 2}},
+			},
+		}
+
+		override := map[string]interface{}{
+			"list": []any{
+				map[string]any{"name": "a"},
+			},
+		}
+
+		result := MergeMaps(base, override)
+
+		// Expected: list should be completely replaced
+		resultList := result["list"].([]any)
+		if len(resultList) != 1 {
+			t.Fatalf("Expected list length 1, got %d", len(resultList))
+		}
+
+		elem := resultList[0].(map[string]any)
+		if elem["name"] != "a" {
+			t.Errorf("Expected name 'a', got %v", elem["name"])
+		}
+		// values field should not exist since it was not in override
+		if _, exists := elem["values"]; exists {
+			t.Errorf("values field should not exist in result, but got: %v", elem["values"])
+		}
+	})
+
+	t.Run("empty array should replace non-empty array", func(t *testing.T) {
+		base := map[string]interface{}{
+			"list": []any{
+				map[string]any{"name": "dummy", "values": []any{1, 2}},
+			},
+		}
+
+		override := map[string]interface{}{
+			"list": []any{},
+		}
+
+		result := MergeMaps(base, override)
+
+		// Expected: list should be empty
+		resultList := result["list"].([]any)
+		if len(resultList) != 0 {
+			t.Errorf("Expected empty list, got length %d: %+v", len(resultList), resultList)
+		}
+	})
+
+	t.Run("nested empty array should replace non-empty nested array", func(t *testing.T) {
+		base := map[string]interface{}{
+			"list": []any{
+				map[string]any{"name": "dummy", "values": []any{1, 2}},
+			},
+		}
+
+		override := map[string]interface{}{
+			"list": []any{
+				map[string]any{"name": "a", "values": []any{}},
+			},
+		}
+
+		result := MergeMaps(base, override)
+
+		// Expected: list should be replaced with one element that has empty values
+		resultList := result["list"].([]any)
+		if len(resultList) != 1 {
+			t.Fatalf("Expected list length 1, got %d", len(resultList))
+		}
+
+		elem := resultList[0].(map[string]any)
+		if elem["name"] != "a" {
+			t.Errorf("Expected name 'a', got %v", elem["name"])
+		}
+		
+		values := elem["values"].([]any)
+		if len(values) != 0 {
+			t.Errorf("Expected empty values array, got length %d: %+v", len(values), values)
+		}
+	})
+}

pkg/state/create.go

@@ -412,9 +412,9 @@ func (c *StateCreator) loadEnvValues(st *HelmState, name string, failOnMissingEn
 	if overrode != nil {
 		intOverrodeEnv := *newEnv
 
-		// Use MergeMaps instead of mergo.Merge to properly handle array merging element-by-element
-		// This fixes issue #2281 where arrays were being replaced entirely instead of merged
-		intOverrodeEnv.Values = maputil.MergeMaps(intOverrodeEnv.Values, overrode.Values)
+		// Use MergeMapsWithArrayMerge instead of mergo.Merge to properly handle array merging element-by-element
+		// This fixes issue #2281 where arrays from --state-values-set were being replaced entirely instead of merged
+		intOverrodeEnv.Values = maputil.MergeMapsWithArrayMerge(intOverrodeEnv.Values, overrode.Values)
 
 		newEnv = &intOverrodeEnv
 	}

pkg/state/state.go

@@ -150,6 +150,9 @@ type SubHelmfileSpec struct {
 // SubhelmfileEnvironmentSpec is the environment spec for a subhelmfile
 type SubhelmfileEnvironmentSpec struct {
 	OverrideValues []any `yaml:"values,omitempty"`
+	// CLISet is set internally to pass CLI overrides from --state-values-set
+	// This is not part of the YAML spec
+	CLISet map[string]any `yaml:"-"`
 }
 
 // HelmSpec to defines helmDefault values