Cleanup and unit tests for util package (maistra#73)

* Cleanup and unit tests for util package

* Review comments

pkg/util/common.go

@@ -16,27 +16,13 @@ package util
 
 import (
 	"strings"
-
-	"gopkg.in/yaml.v2"
 )
 
 const (
 	// LocalFilePrefix is a prefix for local files.
 	LocalFilePrefix = "file:///"
 )
 
-// Tree is a tree.
-type Tree map[string]interface{}
-
-// String implements the Stringer interface method.
-func (t Tree) String() string {
-	y, err := yaml.Marshal(t)
-	if err != nil {
-		return err.Error()
-	}
-	return string(y)
-}
-
 // IsFilePath reports whether the given URL is a local file path.
 func IsFilePath(path string) bool {
 	return strings.HasPrefix(path, LocalFilePrefix)

pkg/util/path.go

@@ -22,12 +22,11 @@ import (
 )
 
 const (
-	kvSeparatorRune = ':'
-
 	// PathSeparator is the separator between path elements.
 	PathSeparator = "."
 	// KVSeparator is the separator between the key and value in a key/value path element,
-	KVSeparator = string(kvSeparatorRune)
+	KVSeparator     = string(kvSeparatorRune)
+	kvSeparatorRune = ':'
 )
 
 var (
@@ -67,10 +66,6 @@ func ToYAMLPathString(path string) string {
 	return p.String()
 }
 
-func firstCharToLowerCase(s string) string {
-	return strings.ToLower(s[0:1]) + s[1:]
-}
-
 // IsValidPathElement reports whether pe is a valid path element.
 func IsValidPathElement(pe string) bool {
 	return ValidKeyRegex.MatchString(pe)
@@ -148,3 +143,7 @@ func splitEscaped(s string, r rune) []string {
 	out = append(out, s[prevIdx:])
 	return out
 }
+
+func firstCharToLowerCase(s string) string {
+	return strings.ToLower(s[0:1]) + s[1:]
+}

pkg/util/reflect.go

@@ -209,21 +209,6 @@ func IsEmptyString(value interface{}) bool {
 	return false
 }
 
-// AppendToSlicePtr inserts value into parent which must be a slice ptr.
-func AppendToSlicePtr(parentSlice interface{}, value interface{}) error {
-	dbgPrint("AppendToSlicePtr slice=\n%s\nvalue=\n%v", pretty.Sprint(parentSlice), value)
-	pv := reflect.ValueOf(parentSlice)
-	v := reflect.ValueOf(value)
-
-	if !IsSliceInterfacePtr(parentSlice) {
-		return fmt.Errorf("appendToSlicePtr parent type is %T, must be *[]interface{}", parentSlice)
-	}
-
-	pv.Elem().Set(reflect.Append(pv.Elem(), v))
-
-	return nil
-}
-
 // DeleteFromSlicePtr deletes an entry at index from the parent, which must be a slice ptr.
 func DeleteFromSlicePtr(parentSlice interface{}, index int) error {
 	dbgPrint("DeleteFromSlicePtr index=%d, slice=\n%s", index, pretty.Sprint(parentSlice))
@@ -238,8 +223,7 @@ func DeleteFromSlicePtr(parentSlice interface{}, index int) error {
 		pvv = pvv.Elem()
 	}
 
-	ns := reflect.AppendSlice(pvv.Slice(0, index), pvv.Slice(index+1, pvv.Len()))
-	pv.Elem().Set(ns)
+	pv.Elem().Set(reflect.AppendSlice(pvv.Slice(0, index), pvv.Slice(index+1, pvv.Len())))
 
 	return nil
 }
@@ -291,7 +275,7 @@ func InsertIntoMap(parentMap interface{}, key interface{}, value interface{}) er
 // ToIntValue returns 0, false if val is not a number type, otherwise it returns the int value of val.
 func ToIntValue(val interface{}) (int, bool) {
 	if IsValueNil(val) {
-		return 0, false
+		return 0, true
 	}
 	v := reflect.ValueOf(val)
 	switch v.Kind() {

pkg/util/reflect_test.go

@@ -304,6 +304,42 @@ func isInListOfInterface(lv []interface{}, v interface{}) bool {
 	return false
 }
 
+func TestDeleteFromSlicePtr(t *testing.T) {
+	parentSlice := []int{42, 43, 44, 45}
+	var parentSliceI interface{} = parentSlice
+	if err := DeleteFromSlicePtr(&parentSliceI, 1); err != nil {
+		t.Fatalf("got error: %s, want error: nil", err)
+	}
+	wantSlice := []int{42, 44, 45}
+	if got, want := parentSliceI, wantSlice; !reflect.DeepEqual(got, want) {
+		t.Errorf("got:\n%v\nwant:\n%v\n", got, want)
+	}
+
+	badParent := struct{}{}
+	wantErr := `deleteFromSlicePtr parent type is *struct {}, must be *[]interface{}`
+	if got, want := errToString(DeleteFromSlicePtr(&badParent, 1)), wantErr; got != want {
+		t.Fatalf("got error: %s, want error: %s", got, want)
+	}
+}
+
+func TestUpdateSlicePtr(t *testing.T) {
+	parentSlice := []int{42, 43, 44, 45}
+	var parentSliceI interface{} = parentSlice
+	if err := UpdateSlicePtr(&parentSliceI, 1, 42); err != nil {
+		t.Fatalf("got error: %s, want error: nil", err)
+	}
+	wantSlice := []int{42, 42, 44, 45}
+	if got, want := parentSliceI, wantSlice; !reflect.DeepEqual(got, want) {
+		t.Errorf("got:\n%v\nwant:\n%v\n", got, want)
+	}
+
+	badParent := struct{}{}
+	wantErr := `updateSlicePtr parent type is *struct {}, must be *[]interface{}`
+	if got, want := errToString(UpdateSlicePtr(&badParent, 1, 42)), wantErr; got != want {
+		t.Fatalf("got error: %s, want error: %s", got, want)
+	}
+}
+
 func TestInsertIntoMap(t *testing.T) {
 	parentMap := map[int]string{42: "forty two", 43: "forty three"}
 	key := 44
@@ -322,3 +358,48 @@ func TestInsertIntoMap(t *testing.T) {
 		t.Fatalf("got error: %s, want error: %s", got, want)
 	}
 }
+
+func TestToIntValue(t *testing.T) {
+	tests := []struct {
+		desc   string
+		in     interface{}
+		wantOk bool
+		want   int
+	}{
+		{
+			desc:   "nil success",
+			in:     nil,
+			wantOk: true,
+			want:   0,
+		},
+		{
+			desc:   "int8 success",
+			in:     int8(-42),
+			wantOk: true,
+			want:   -42,
+		},
+		{
+			desc:   "uint32 success",
+			in:     uint32(42),
+			wantOk: true,
+			want:   42,
+		},
+		{
+			desc:   "bed type fail",
+			in:     "42",
+			wantOk: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			got, ok := ToIntValue(tt.in)
+			if ok != tt.wantOk {
+				t.Errorf("%s: gotOk %v, wantOk %v", tt.desc, ok, tt.wantOk)
+			}
+			if got != tt.want {
+				t.Errorf("%s: got %v, want %v", tt.desc, got, tt.want)
+			}
+		})
+	}
+}

pkg/validate/validate_values.go

@@ -29,7 +29,7 @@ var (
 )
 
 // CheckValues validates the values in the given tree, which follows the Istio values.yaml schema.
-func CheckValues(root util.Tree) util.Errors {
+func CheckValues(root map[string]interface{}) util.Errors {
 	return validateValues(defaultValuesValidations, root, nil)
 }
 
@@ -41,13 +41,10 @@ func validateValues(validations map[string]ValidatorFunc, node interface{}, path
 		errs = util.AppendErrs(errs, vf(path, node))
 	}
 
-	nn, ok := node.(util.Tree)
+	nn, ok := node.(map[string]interface{})
 	if !ok {
-		nn, ok = node.(map[string]interface{})
-		if !ok {
-			// Leaf, nothing more to recurse.
-			return
-		}
+		// Leaf, nothing more to recurse.
+		return errs
 	}
 	for k, v := range nn {
 		errs = util.AppendErrs(errs, validateValues(validations, v, append(path, k)))

pkg/validate/validate_values_test.go

@@ -98,7 +98,7 @@ global:
 
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
-			root := util.Tree{}
+			root := make(map[string]interface{})
 			err := yaml.Unmarshal([]byte(tt.yamlStr), &root)
 			if err != nil {
 				t.Fatalf("yaml.Unmarshal(%s): got error %s", tt.desc, err)