Fix map merging logic

Fixes #777

Signed-off-by: Nolan Brubaker <nolan@heptio.com>

pkg/restore/merge_service_account.go

@@ -58,9 +58,9 @@ func mergeServiceAccounts(fromCluster, fromBackup *unstructured.Unstructured) (*
 
 	desired.ImagePullSecrets = mergeLocalObjectReferenceSlices(desired.ImagePullSecrets, backupSA.ImagePullSecrets)
 
-	collections.MergeMaps(desired.Labels, backupSA.Labels)
+	desired.Labels = collections.MergeMaps(desired.Labels, backupSA.Labels)
 
-	collections.MergeMaps(desired.Annotations, backupSA.Annotations)
+	desired.Annotations = collections.MergeMaps(desired.Annotations, backupSA.Annotations)
 
 	desiredUnstructured, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desired)
 	if err != nil {

pkg/util/collections/map_utils.go

@@ -125,11 +125,18 @@ func Exists(root map[string]interface{}, path string) bool {
 
 // MergeMaps takes two map[string]string and merges missing keys from the second into the first.
 // If a key already exists, its value is not overwritten.
-func MergeMaps(first, second map[string]string) {
+func MergeMaps(first, second map[string]string) map[string]string {
+	// If the first map passed in is empty, just use all of the second map's data
+	if first == nil {
+		first = map[string]string{}
+	}
+
 	for k, v := range second {
 		_, ok := first[k]
 		if !ok {
 			first[k] = v
 		}
 	}
+
+	return first
 }

pkg/util/collections/map_utils_test.go

@@ -18,6 +18,8 @@ package collections
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestGetString(t *testing.T) {
@@ -44,3 +46,57 @@ func TestGetString(t *testing.T) {
 		}
 	}
 }
+
+func TestMergeMaps(t *testing.T) {
+	var testCases = []struct {
+		name        string
+		source      map[string]string
+		destination map[string]string
+		expected    map[string]string
+	}{
+		{
+			name:        "nil destination should result in source being copied",
+			destination: nil,
+			source: map[string]string{
+				"k1": "v1",
+			},
+			expected: map[string]string{
+				"k1": "v1",
+			},
+		},
+		{
+			name: "keys missing from destination should be copied from source",
+			destination: map[string]string{
+				"k2": "v2",
+			},
+			source: map[string]string{
+				"k1": "v1",
+			},
+			expected: map[string]string{
+				"k1": "v1",
+				"k2": "v2",
+			},
+		},
+		{
+			name: "matching key should not have value copied from source",
+			destination: map[string]string{
+				"k1": "v1",
+			},
+			source: map[string]string{
+				"k1": "v2",
+			},
+			expected: map[string]string{
+				"k1": "v1",
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			result := MergeMaps(tc.destination, tc.source)
+
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}