fixup! refactor: Move merge function to common module

Author: jimmidyson

api/go.mod

@@ -25,6 +25,7 @@ require (
 )
 
 require (
+	dario.cat/mergo v1.0.1 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect

api/go.sum

@@ -1,3 +1,5 @@
+dario.cat/mergo v1.0.1 h1:Ra4+bf83h2ztPIQYNP99R6m+Y7KfnARDfID+a+vLl4s=
+dario.cat/mergo v1.0.1/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk=
 github.com/aws/aws-sdk-go v1.55.8 h1:JRmEUbU52aJQZ2AjX4q4Wu7t4uZjOu71uyNmaWlUkJQ=
 github.com/aws/aws-sdk-go v1.55.8/go.mod h1:ZkViS9AqA6otK+JBBNH2++sx1sgxrPKcSzPPvQkUtXk=
 github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=

common/pkg/capi/clustertopology/handlers/mutation/meta.go

@@ -5,12 +5,9 @@ package mutation
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
-	"maps"
 	"sync"
 
-	"dario.cat/mergo"
 	"github.com/samber/lo"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -22,6 +19,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/capi/clustertopology/handlers"
+	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/capi/clustertopology/variables"
 )
 
 type MutateFunc func(
@@ -130,7 +128,7 @@ func (mgp metaGeneratePatches) GeneratePatches(
 			// Merge the global variables to the current resource vars. This allows the handlers to access
 			// the variables defined in the cluster class or cluster configuration and use these correctly when
 			// overrides are specified on machine deployment or control plane configuration.
-			mergedVars, err := mergeVariableOverridesWithGlobal(vars, globalVars)
+			mergedVars, err := variables.MergeVariableOverridesWithGlobal(vars, globalVars)
 			if err != nil {
 				log.Error(err, "Failed to merge global variables")
 				return err
@@ -161,56 +159,3 @@ func (mgp metaGeneratePatches) GeneratePatches(
 		},
 	)
 }
-
-// mergeVariableOverridesWithGlobal merges the provided variable overrides with the global variables.
-// It performs a deep merge, ensuring that if a variable exists in both maps, the value from the overrides is used.
-func mergeVariableOverridesWithGlobal(
-	overrideVars, globalVars map[string]apiextensionsv1.JSON,
-) (map[string]apiextensionsv1.JSON, error) {
-	mergedVars := maps.Clone(overrideVars)
-
-	for k, v := range globalVars {
-		// If the value of v is nil, skip it.
-		if v.Raw == nil {
-			continue
-		}
-
-		existingValue, exists := mergedVars[k]
-
-		// If the variable does not exist in the mergedVars or the value is nil, add it and continue.
-		if !exists || existingValue.Raw == nil {
-			mergedVars[k] = v
-			continue
-		}
-
-		// Wrap the value in a temporary key to ensure we can unmarshal to a map.
-		// This is necessary because the values could be scalars.
-		tempValJSON := fmt.Sprintf(`{"value": %s}`, string(existingValue.Raw))
-		tempGlobalValJSON := fmt.Sprintf(`{"value": %s}`, string(v.Raw))
-
-		// Unmarshal the existing value and the global value into maps.
-		var val, globalVal map[string]interface{}
-		if err := json.Unmarshal([]byte(tempValJSON), &val); err != nil {
-			return nil, fmt.Errorf("failed to unmarshal existing value for key %q: %w", k, err)
-		}
-
-		if err := json.Unmarshal([]byte(tempGlobalValJSON), &globalVal); err != nil {
-			return nil, fmt.Errorf("failed to unmarshal global value for key %q: %w", k, err)
-		}
-
-		// Now use mergo to perform a deep merge of the values, retaining the values in `val` if present.
-		if err := mergo.Merge(&val, globalVal); err != nil {
-			return nil, fmt.Errorf("failed to merge values for key %q: %w", k, err)
-		}
-
-		// Marshal the merged value back to JSON.
-		mergedVal, err := json.Marshal(val["value"])
-		if err != nil {
-			return nil, fmt.Errorf("failed to marshal merged value for key %q: %w", k, err)
-		}
-
-		mergedVars[k] = apiextensionsv1.JSON{Raw: mergedVal}
-	}
-
-	return mergedVars, nil
-}

common/pkg/capi/clustertopology/handlers/mutation/meta_test.go

@@ -289,198 +289,3 @@ func jsonPatch(operations ...jsonpatch.Operation) []byte {
 	)
 	return b
 }
-
-func TestMergeVariableDefinitions(t *testing.T) {
-	t.Parallel()
-
-	type args struct {
-		vars       map[string]apiextensionsv1.JSON
-		globalVars map[string]apiextensionsv1.JSON
-	}
-	tests := []struct {
-		name      string
-		args      args
-		want      map[string]apiextensionsv1.JSON
-		wantErr   bool
-		errString string
-	}{
-		{
-			name: "no overlap, globalVars added",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`1`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"b": {Raw: []byte(`2`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`1`)},
-				"b": {Raw: []byte(`2`)},
-			},
-		},
-		{
-			name: "globalVars value is nil, skipped",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`1`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"b": {Raw: nil},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`1`)},
-			},
-		},
-		{
-			name: "existing value is nil, globalVars value used",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: nil},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`2`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`2`)},
-			},
-		},
-		{
-			name: "both values are scalars, globalVars ignored",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`1`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`2`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`1`)},
-			},
-		},
-		{
-			name: "both values are objects, merged",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"x":1,"y":2}`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"y":3,"z":4}`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`{"x":1,"y":2,"z":4}`)},
-			},
-		},
-		{
-			name: "both values are objects with nested objects, merged",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"x":1,"y":{"a": 2,"b":{"c": 3}}}`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"y":{"a": 2,"b":{"c": 5, "d": 6}},"z":4}`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`{"x":1,"y":{"a": 2,"b":{"c": 3, "d": 6}},"z":4}`)},
-			},
-		},
-		{
-			name: "both values are objects with nested objects with vars having nil object explicitly, merged",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"x":1,"y":{"a": 2,"b": null}}`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"y":{"a": 2,"b":{"c": 5, "d": 6}},"z":4}`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`{"x":1,"y":{"a": 2,"b":{"c": 5, "d": 6}},"z":4}`)},
-			},
-		},
-		{
-			name: "globalVars is scalar, vars is object, keep object",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"x":1}`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`2`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`{"x":1}`)},
-			},
-		},
-		{
-			name: "vars is scalar, globalVars is object, keep scalar",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`2`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"x":1}`)},
-				},
-			},
-			want: map[string]apiextensionsv1.JSON{
-				"a": {Raw: []byte(`2`)},
-			},
-		},
-		{
-			name: "invalid JSON in vars",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{invalid}`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"x":1}`)},
-				},
-			},
-			wantErr:   true,
-			errString: "failed to unmarshal existing value for key \"a\"",
-		},
-		{
-			name: "invalid JSON in globalVars",
-			args: args{
-				vars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{"x":1}`)},
-				},
-				globalVars: map[string]apiextensionsv1.JSON{
-					"a": {Raw: []byte(`{invalid}`)},
-				},
-			},
-			wantErr:   true,
-			errString: "failed to unmarshal global value for key \"a\"",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			g := gomega.NewWithT(t)
-			got, err := mergeVariableOverridesWithGlobal(tt.args.vars, tt.args.globalVars)
-			if tt.wantErr {
-				g.Expect(err).To(gomega.HaveOccurred())
-				g.Expect(err.Error()).To(gomega.ContainSubstring(tt.errString))
-				return
-			}
-			g.Expect(err).ToNot(gomega.HaveOccurred())
-			// Compare JSON values
-			for k, wantVal := range tt.want {
-				gotVal, ok := got[k]
-				g.Expect(ok).To(gomega.BeTrue(), "missing key %q", k)
-				var wantObj, gotObj interface{}
-				_ = json.Unmarshal(wantVal.Raw, &wantObj)
-				_ = json.Unmarshal(gotVal.Raw, &gotObj)
-				g.Expect(gotObj).To(gomega.Equal(wantObj), "key %q", k)
-			}
-			// Check for unexpected keys
-			g.Expect(len(got)).To(gomega.Equal(len(tt.want)))
-		})
-	}
-}

common/pkg/capi/clustertopology/variables/merge.go

@@ -0,0 +1,66 @@
+// Copyright 2023 Nutanix. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package variables
+
+import (
+	"encoding/json"
+	"fmt"
+	"maps"
+
+	"dario.cat/mergo"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+// MergeVariableOverridesWithGlobal merges the provided variable overrides with the global variables.
+// It performs a deep merge, ensuring that if a variable exists in both maps, the value from the overrides is used.
+func MergeVariableOverridesWithGlobal(
+	overrideVars, globalVars map[string]apiextensionsv1.JSON,
+) (map[string]apiextensionsv1.JSON, error) {
+	mergedVars := maps.Clone(overrideVars)
+
+	for k, v := range globalVars {
+		// If the value of v is nil, skip it.
+		if v.Raw == nil {
+			continue
+		}
+
+		existingValue, exists := mergedVars[k]
+
+		// If the variable does not exist in the mergedVars or the value is nil, add it and continue.
+		if !exists || existingValue.Raw == nil {
+			mergedVars[k] = v
+			continue
+		}
+
+		// Wrap the value in a temporary key to ensure we can unmarshal to a map.
+		// This is necessary because the values could be scalars.
+		tempValJSON := fmt.Sprintf(`{"value": %s}`, string(existingValue.Raw))
+		tempGlobalValJSON := fmt.Sprintf(`{"value": %s}`, string(v.Raw))
+
+		// Unmarshal the existing value and the global value into maps.
+		var val, globalVal map[string]interface{}
+		if err := json.Unmarshal([]byte(tempValJSON), &val); err != nil {
+			return nil, fmt.Errorf("failed to unmarshal existing value for key %q: %w", k, err)
+		}
+
+		if err := json.Unmarshal([]byte(tempGlobalValJSON), &globalVal); err != nil {
+			return nil, fmt.Errorf("failed to unmarshal global value for key %q: %w", k, err)
+		}
+
+		// Now use mergo to perform a deep merge of the values, retaining the values in `val` if present.
+		if err := mergo.Merge(&val, globalVal); err != nil {
+			return nil, fmt.Errorf("failed to merge values for key %q: %w", k, err)
+		}
+
+		// Marshal the merged value back to JSON.
+		mergedVal, err := json.Marshal(val["value"])
+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal merged value for key %q: %w", k, err)
+		}
+
+		mergedVars[k] = apiextensionsv1.JSON{Raw: mergedVal}
+	}
+
+	return mergedVars, nil
+}