Merge pull request #4784 from cli/config-map-fixes

Fix up bug in RemoveEntry and add tests for config_map

internal/config/config_map.go

@@ -6,7 +6,7 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
-// This type implements a low-level get/set config that is backed by an in-memory tree of Yaml
+// This type implements a low-level get/set config that is backed by an in-memory tree of yaml
 // nodes. It allows us to interact with a yaml-based config programmatically, preserving any
 // comments that were present when the yaml was parsed.
 type ConfigMap struct {
@@ -37,41 +37,41 @@ func (cm *ConfigMap) GetStringValue(key string) (string, error) {
 
 func (cm *ConfigMap) SetStringValue(key, value string) error {
 	entry, err := cm.FindEntry(key)
+	if err == nil {
+		entry.ValueNode.Value = value
+		return nil
+	}
 
 	var notFound *NotFoundError
-
-	valueNode := entry.ValueNode
-
-	if err != nil && errors.As(err, &notFound) {
-		keyNode := &yaml.Node{
-			Kind:  yaml.ScalarNode,
-			Value: key,
-		}
-		valueNode = &yaml.Node{
-			Kind:  yaml.ScalarNode,
-			Tag:   "!!str",
-			Value: "",
-		}
-
-		cm.Root.Content = append(cm.Root.Content, keyNode, valueNode)
-	} else if err != nil {
+	if err != nil && !errors.As(err, &notFound) {
 		return err
 	}
 
-	valueNode.Value = value
+	keyNode := &yaml.Node{
+		Kind:  yaml.ScalarNode,
+		Value: key,
+	}
+	valueNode := &yaml.Node{
+		Kind:  yaml.ScalarNode,
+		Tag:   "!!str",
+		Value: value,
+	}
 
+	cm.Root.Content = append(cm.Root.Content, keyNode, valueNode)
 	return nil
 }
 
-func (cm *ConfigMap) FindEntry(key string) (ce *ConfigEntry, err error) {
-	err = nil
+func (cm *ConfigMap) FindEntry(key string) (*ConfigEntry, error) {
+	ce := &ConfigEntry{}
 
-	ce = &ConfigEntry{}
+	if cm.Empty() {
+		return ce, &NotFoundError{errors.New("not found")}
+	}
 
-	// Content slice goes [key1, value1, key2, value2, ...]
+	// Content slice goes [key1, value1, key2, value2, ...].
 	topLevelPairs := cm.Root.Content
 	for i, v := range topLevelPairs {
-		// Skip every other slice item since we only want to check against keys
+		// Skip every other slice item since we only want to check against keys.
 		if i%2 != 0 {
 			continue
 		}
@@ -81,7 +81,7 @@ func (cm *ConfigMap) FindEntry(key string) (ce *ConfigEntry, err error) {
 			if i+1 < len(topLevelPairs) {
 				ce.ValueNode = topLevelPairs[i+1]
 			}
-			return
+			return ce, nil
 		}
 	}
 
@@ -89,14 +89,23 @@ func (cm *ConfigMap) FindEntry(key string) (ce *ConfigEntry, err error) {
 }
 
 func (cm *ConfigMap) RemoveEntry(key string) {
+	if cm.Empty() {
+		return
+	}
+
 	newContent := []*yaml.Node{}
 
-	content := cm.Root.Content
-	for i := 0; i < len(content); i++ {
-		if content[i].Value == key {
-			i++ // skip the next node which is this key's value
+	var skipNext bool
+	for i, v := range cm.Root.Content {
+		if skipNext {
+			skipNext = false
+			continue
+		}
+		if i%2 != 0 || v.Value != key {
+			newContent = append(newContent, v)
 		} else {
-			newContent = append(newContent, content[i])
+			// Don't append current node and skip the next which is this key's value.
+			skipNext = true
 		}
 	}
 

internal/config/config_map_test.go

@@ -1,7 +1,6 @@
 package config
 
 import (
-	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -46,12 +45,135 @@ func TestFindEntry(t *testing.T) {
 				return
 			}
 			assert.NoError(t, err)
-			fmt.Println(out)
 			assert.Equal(t, tt.output, out.ValueNode.Value)
 		})
 	}
 }
 
+func TestEmpty(t *testing.T) {
+	cm := ConfigMap{}
+	assert.Equal(t, true, cm.Empty())
+	cm.Root = &yaml.Node{
+		Content: []*yaml.Node{
+			{
+				Value: "test",
+			},
+		},
+	}
+	assert.Equal(t, false, cm.Empty())
+}
+
+func TestGetStringValue(t *testing.T) {
+	tests := []struct {
+		name      string
+		key       string
+		wantValue string
+		wantErr   bool
+	}{
+		{
+			name:      "get key",
+			key:       "valid",
+			wantValue: "present",
+		},
+		{
+			name:    "get key that is not present",
+			key:     "invalid",
+			wantErr: true,
+		},
+		{
+			name:      "get key that has same content as a value",
+			key:       "same",
+			wantValue: "logical",
+		},
+	}
+
+	for _, tt := range tests {
+		cm := ConfigMap{Root: testYaml()}
+		t.Run(tt.name, func(t *testing.T) {
+			val, err := cm.GetStringValue(tt.key)
+			if tt.wantErr {
+				assert.EqualError(t, err, "not found")
+				return
+			}
+			assert.Equal(t, tt.wantValue, val)
+		})
+	}
+}
+
+func TestSetStringValue(t *testing.T) {
+	tests := []struct {
+		name  string
+		key   string
+		value string
+	}{
+		{
+			name:  "set key that is not present",
+			key:   "notPresent",
+			value: "test1",
+		},
+		{
+			name:  "set key that is present",
+			key:   "erroneous",
+			value: "test2",
+		},
+		{
+			name:  "set key that is blank",
+			key:   "blank",
+			value: "test3",
+		},
+		{
+			name:  "set key that has same content as a value",
+			key:   "present",
+			value: "test4",
+		},
+	}
+
+	for _, tt := range tests {
+		cm := ConfigMap{Root: testYaml()}
+		t.Run(tt.name, func(t *testing.T) {
+			err := cm.SetStringValue(tt.key, tt.value)
+			assert.NoError(t, err)
+			val, err := cm.GetStringValue(tt.key)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.value, val)
+		})
+	}
+}
+
+func TestRemoveEntry(t *testing.T) {
+	tests := []struct {
+		name       string
+		key        string
+		wantLength int
+	}{
+		{
+			name:       "remove key",
+			key:        "erroneous",
+			wantLength: 6,
+		},
+		{
+			name:       "remove key that is not present",
+			key:        "invalid",
+			wantLength: 8,
+		},
+		{
+			name:       "remove key that has same content as a value",
+			key:        "same",
+			wantLength: 6,
+		},
+	}
+
+	for _, tt := range tests {
+		cm := ConfigMap{Root: testYaml()}
+		t.Run(tt.name, func(t *testing.T) {
+			cm.RemoveEntry(tt.key)
+			assert.Equal(t, tt.wantLength, len(cm.Root.Content))
+			_, err := cm.FindEntry(tt.key)
+			assert.EqualError(t, err, "not found")
+		})
+	}
+}
+
 func testYaml() *yaml.Node {
 	var root yaml.Node
 	var data = `