From 85b4eef973d73ed9213b7ef76a404f2da7216dd3 Mon Sep 17 00:00:00 2001 From: Adam Eijdenberg Date: Fri, 18 Jan 2019 15:25:30 +1100 Subject: [PATCH] Fix nested null value overrides - Add ability to test for nested non-existent keys - Add test cases for nested null values - Minimalist fix for nested null key test cases - Add missing metadata to integration test Signed-off-by: Adam Eijdenberg (cherry picked from commit 5b9311d163654c2d3a7ee54742f6497a017a91ee) --- cmd/helm/get_values_test.go | 7 +++- pkg/chartutil/testdata/moby/values.yaml | 15 +++++++ pkg/chartutil/values.go | 19 +++++++-- pkg/chartutil/values_test.go | 54 +++++++++++++++++++++++-- 4 files changed, 85 insertions(+), 10 deletions(-) diff --git a/cmd/helm/get_values_test.go b/cmd/helm/get_values_test.go index aec5ce0c2..40b46bfda 100644 --- a/cmd/helm/get_values_test.go +++ b/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"`}, }) diff --git a/pkg/chartutil/testdata/moby/values.yaml b/pkg/chartutil/testdata/moby/values.yaml index 54e1ce463..ecf22b563 100644 --- a/pkg/chartutil/testdata/moby/values.yaml +++ b/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 diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go index 352524c13..6270531d7 100644 --- a/pkg/chartutil/values.go +++ b/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 } diff --git a/pkg/chartutil/values_test.go b/pkg/chartutil/values_test.go index 3fea14c3a..af6c77d27 100644 --- a/pkg/chartutil/values_test.go +++ b/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}}", ""}, + + {"{{.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"])