Merge pull request #5185 from govau/fixnestednullcoalesce

Fix nested null value overrides

cmd/helm/get_values_test.go

@@ -29,8 +29,11 @@ import (
 
 func TestGetValuesCmd(t *testing.T) {
 	releaseWithValues := helm.ReleaseMock(&helm.MockReleaseOptions{
-		Name:   "thomas-guide",
-		Chart:  &chart.Chart{Values: &chart.Config{Raw: `foo2: "bar2"`}},
+		Name: "thomas-guide",
+		Chart: &chart.Chart{
+			Metadata: &chart.Metadata{Name: "thomas-guide-chart-name"},
+			Values:   &chart.Config{Raw: `foo2: "bar2"`},
+		},
 		Config: &chart.Config{Raw: `foo: "bar"`},
 	})
 

pkg/chartutil/testdata/moby/values.yaml

@@ -7,3 +7,18 @@ right: exists
 left: exists
 front: exists
 back: exists
+
+# nested tables for null coalesce testing
+web:
+  livenessProbe:
+    failureThreshold: 5
+    httpGet:
+      path: /api/v1/info
+      port: atc
+    initialDelaySeconds: 10
+    periodSeconds: 15
+    timeoutSeconds: 3
+  readinessProbe:
+    httpGet:
+      path: /api/v1/info
+      port: atc

pkg/chartutil/values.go

@@ -327,16 +327,21 @@ func coalesceTables(dst, src map[string]interface{}, chartName string) map[strin
 	// Because dest has higher precedence than src, dest values override src
 	// values.
 	for key, val := range src {
+		dv, ok := dst[key]
+		if ok && dv == nil {
+			// skip here, we delete at end
+			continue
+		}
 		if istable(val) {
-			if innerdst, ok := dst[key]; !ok {
+			if !ok {
 				dst[key] = val
-			} else if istable(innerdst) {
-				coalesceTables(innerdst.(map[string]interface{}), val.(map[string]interface{}), chartName)
+			} else if istable(dv) {
+				coalesceTables(dv.(map[string]interface{}), val.(map[string]interface{}), chartName)
 			} else {
 				log.Printf("Warning: Merging destination map for chart '%s'. Cannot overwrite table item '%s', with non table value: %v", chartName, key, val)
 			}
 			continue
-		} else if dv, ok := dst[key]; ok && istable(dv) {
+		} else if ok && istable(dv) {
 			log.Printf("Warning: Merging destination map for chart '%s'. The destination item '%s' is a table and ignoring the source '%s' as it has a non-table value of: %v", chartName, key, key, val)
 			continue
 		} else if !ok { // <- ok is still in scope from preceding conditional.
@@ -344,6 +349,12 @@ func coalesceTables(dst, src map[string]interface{}, chartName string) map[strin
 			continue
 		}
 	}
+	// never return a nil value, rather delete the key
+	for k, v := range dst {
+		if v == nil {
+			delete(dst, k)
+		}
+	}
 	return dst
 }
 

pkg/chartutil/values_test.go

@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 	"text/template"
 
@@ -300,6 +301,25 @@ pequod:
       sail: true
   ahab:
     scope: whale
+
+# test coalesce with nested null values
+web:
+  livenessProbe:
+    httpGet: null
+    exec:
+      command:
+      - curl
+      - -f
+      - http://localhost:8080/api/v1/info
+    timeoutSeconds: null
+  readinessProbe:
+    httpGet: null
+    exec:
+      command:
+      - curl
+      - -f
+      - http://localhost:8080/api/v1/info
+    timeoutSeconds: null # catches the case where this wasn't defined in the original source...
 `
 
 func TestCoalesceValues(t *testing.T) {
@@ -344,6 +364,13 @@ func TestCoalesceValues(t *testing.T) {
 		{"{{.spouter.global.nested.boat}}", "true"},
 		{"{{.pequod.global.nested.sail}}", "true"},
 		{"{{.spouter.global.nested.sail}}", "<no value>"},
+
+		{"{{.web.livenessProbe.failureThreshold}}", "5"},
+		{"{{.web.livenessProbe.initialDelaySeconds}}", "10"},
+		{"{{.web.livenessProbe.periodSeconds}}", "15"},
+		{"{{.web.livenessProbe.exec}}", "map[command:[curl -f http://localhost:8080/api/v1/info]]"},
+
+		{"{{.web.readinessProbe.exec}}", "map[command:[curl -f http://localhost:8080/api/v1/info]]"},
 	}
 
 	for _, tt := range tests {
@@ -352,10 +379,29 @@ func TestCoalesceValues(t *testing.T) {
 		}
 	}
 
-	nullKeys := []string{"bottom", "right", "left", "front"}
+	nullKeys := []string{"bottom", "right", "left", "front",
+		"web.livenessProbe.httpGet", "web.readinessProbe.httpGet", "web.livenessProbe.timeoutSeconds", "web.readinessProbe.timeoutSeconds"}
 	for _, nullKey := range nullKeys {
-		if _, ok := v[nullKey]; ok {
-			t.Errorf("Expected key %q to be removed, still present", nullKey)
+		parts := strings.Split(nullKey, ".")
+		curMap := v
+		for partIdx, part := range parts {
+			nextVal, ok := curMap[part]
+			if partIdx == len(parts)-1 { // are we the last?
+				if ok {
+					t.Errorf("Expected key %q to be removed, still present", nullKey)
+					break
+				}
+			} else { // we are not the last
+				if !ok {
+					t.Errorf("Expected key %q to be removed, but partial parent path was not found", nullKey)
+					break
+				}
+				curMap, ok = nextVal.(map[string]interface{})
+				if !ok {
+					t.Errorf("Expected key %q to be removed, but partial parent path did not result in a map", nullKey)
+					break
+				}
+			}
 		}
 	}
 }
@@ -386,7 +432,7 @@ func TestCoalesceTables(t *testing.T) {
 
 	// What we expect is that anything in dst overrides anything in src, but that
 	// otherwise the values are coalesced.
-	coalesceTables(dst, src, "")
+	dst = coalesceTables(dst, src, "")
 
 	if dst["name"] != "Ishmael" {
 		t.Errorf("Unexpected name: %s", dst["name"])