From 95eeba3805d5105f33228328a78e8bf73b406776 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Thu, 21 Jul 2016 15:55:35 -0600 Subject: [PATCH] fix(tiller): merge -f values correctly This fixes a bug in which passed-in values files were not correctly merged into the chart's default values YAML data. I believe it also fixes some other prioritization bugs in values merging. The existing unit test was wrong (see TestCoalesceValues). It is fixed now. Also added more tests to simulate issue #971. In the course of writing this, I removed some vestigial code as mentioned in #920. Closes #971 Closes #920 --- cmd/helm/get.go | 2 +- cmd/helm/get_values.go | 2 +- pkg/chartutil/values.go | 37 ++++++++++--------- pkg/chartutil/values_test.go | 69 ++++++++++++++++++++++++++++++++---- pkg/engine/engine_test.go | 19 +++++----- 5 files changed, 91 insertions(+), 38 deletions(-) diff --git a/cmd/helm/get.go b/cmd/helm/get.go index c98975148..f1ad5334c 100644 --- a/cmd/helm/get.go +++ b/cmd/helm/get.go @@ -101,7 +101,7 @@ func (g *getCmd) run() error { return prettyError(err) } - cfg, err := chartutil.CoalesceValues(res.Release.Chart, res.Release.Config, nil) + cfg, err := chartutil.CoalesceValues(res.Release.Chart, res.Release.Config) if err != nil { return err } diff --git a/cmd/helm/get_values.go b/cmd/helm/get_values.go index bd06e7fb2..c95b6e056 100644 --- a/cmd/helm/get_values.go +++ b/cmd/helm/get_values.go @@ -68,7 +68,7 @@ func (g *getValuesCmd) run() error { // If the user wants all values, compute the values and return. if g.allValues { - cfg, err := chartutil.CoalesceValues(res.Release.Chart, res.Release.Config, nil) + cfg, err := chartutil.CoalesceValues(res.Release.Chart, res.Release.Config) if err != nil { return err } diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go index d135f41df..6585fce8d 100644 --- a/pkg/chartutil/values.go +++ b/pkg/chartutil/values.go @@ -130,8 +130,6 @@ func ReadValuesFile(filename string) (Values, error) { // CoalesceValues coalesces all of the values in a chart (and its subcharts). // -// The overrides map may be used to specifically override configuration values. -// // Values are coalesced together using the following rules: // // - Values in a higher level chart always override values in a lower-level @@ -139,7 +137,7 @@ func ReadValuesFile(filename string) (Values, error) { // - Scalar values and arrays are replaced, maps are merged // - A chart has access to all of the variables for it, as well as all of // the values destined for its dependencies. -func CoalesceValues(chrt *chart.Chart, vals *chart.Config, overrides map[string]interface{}) (Values, error) { +func CoalesceValues(chrt *chart.Chart, vals *chart.Config) (Values, error) { cvals := Values{} // Parse values if not nil. We merge these at the top level because // the passed-in values are in the same namespace as the parent chart. @@ -148,15 +146,7 @@ func CoalesceValues(chrt *chart.Chart, vals *chart.Config, overrides map[string] if err != nil { return cvals, err } - // Override the top-level values. Overrides are NEVER merged deeply. - // The assumption is that an override is intended to set an explicit - // and exact value. - for k, v := range overrides { - evals[k] = v - } - cvals = coalesceValues(chrt, evals) - } else if len(overrides) > 0 { - cvals = coalesceValues(chrt, overrides) + cvals = coalesce(chrt, evals) } cvals = coalesceDeps(chrt, cvals) @@ -255,14 +245,17 @@ func coalesceValues(c *chart.Chart, v map[string]interface{}) map[string]interfa for key, val := range nv { if _, ok := v[key]; !ok { + // If the key is not in v, copy it from nv. v[key] = val } else if dest, ok := v[key].(map[string]interface{}); ok { + // if v[key] is a table, merge nv's val table into v[key]. src, ok := val.(map[string]interface{}) if !ok { log.Printf("warning: skipped value for %s: Not a table.", key) continue } - // coalesce tables + // Because v has higher precedence than nv, dest values override src + // values. coalesceTables(dest, src) } } @@ -270,7 +263,11 @@ func coalesceValues(c *chart.Chart, v map[string]interface{}) map[string]interfa } // coalesceTables merges a source map into a destination map. +// +// dest is considered authoritative. func coalesceTables(dst, src map[string]interface{}) map[string]interface{} { + // Because dest has higher precedence than src, dest values override src + // values. for key, val := range src { if istable(val) { if innerdst, ok := dst[key]; !ok { @@ -284,8 +281,10 @@ func coalesceTables(dst, src map[string]interface{}) map[string]interface{} { } else if dv, ok := dst[key]; ok && istable(dv) { log.Printf("warning: destination for %s is a table. Ignoring non-table value %v", key, val) continue + } else if !ok { // <- ok is still in scope from preceding conditional. + dst[key] = val + continue } - dst[key] = val } return dst } @@ -300,7 +299,7 @@ type ReleaseOptions struct { // ToRenderValues composes the struct from the data coming from the Releases, Charts and Values files func ToRenderValues(chrt *chart.Chart, chrtVals *chart.Config, options ReleaseOptions) (Values, error) { - overrides := map[string]interface{}{ + top := map[string]interface{}{ "Release": map[string]interface{}{ "Name": options.Name, "Time": options.Time, @@ -310,13 +309,13 @@ func ToRenderValues(chrt *chart.Chart, chrtVals *chart.Config, options ReleaseOp "Chart": chrt.Metadata, } - vals, err := CoalesceValues(chrt, chrtVals, nil) + vals, err := CoalesceValues(chrt, chrtVals) if err != nil { - return overrides, err + return top, err } - overrides["Values"] = vals - return overrides, nil + top["Values"] = vals + return top, nil } // istable is a special-purpose function to see if the present thing matches the definition of a YAML table. diff --git a/pkg/chartutil/values_test.go b/pkg/chartutil/values_test.go index 75d0d2e7d..3b58277b8 100644 --- a/pkg/chartutil/values_test.go +++ b/pkg/chartutil/values_test.go @@ -24,6 +24,7 @@ import ( "text/template" "k8s.io/helm/pkg/proto/hapi/chart" + "k8s.io/helm/pkg/timeconv" ) func TestReadValues(t *testing.T) { @@ -67,6 +68,63 @@ water: } } +func TestToRenderValues(t *testing.T) { + + chartValues := ` +name: al Rashid +where: + city: Basrah + title: caliph +` + overideValues := ` +name: Haroun +where: + city: Baghdad + date: 809 CE +` + + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "test"}, + Templates: []*chart.Template{}, + Values: &chart.Config{Raw: chartValues}, + Dependencies: []*chart.Chart{ + { + Metadata: &chart.Metadata{Name: "where"}, + Values: &chart.Config{Raw: ""}, + }, + }, + } + v := &chart.Config{Raw: overideValues} + + o := ReleaseOptions{ + Name: "Seven Voyages", + Time: timeconv.Now(), + Namespace: "al Basrah", + } + + res, err := ToRenderValues(c, v, o) + if err != nil { + t.Fatal(err) + } + var vals Values + vals = res["Values"].(Values) + + if vals["name"] != "Haroun" { + t.Errorf("Expected 'Haroun', got %q (%v)", vals["name"], vals) + } + where := vals["where"].(map[string]interface{}) + expects := map[string]string{ + "city": "Baghdad", + "date": "809 CE", + "title": "caliph", + } + for field, expect := range expects { + if got := where[field]; got != expect { + t.Errorf("Expected %q, got %q (%v)", expect, got, where) + } + } +} + func TestReadValuesFile(t *testing.T) { data, err := ReadValuesFile("./testdata/coleridge.yaml") if err != nil { @@ -188,9 +246,6 @@ pequod: func TestCoalesceValues(t *testing.T) { tchart := "testdata/moby" - overrides := map[string]interface{}{ - "override": "good", - } c, err := LoadDir(tchart) if err != nil { t.Fatal(err) @@ -198,7 +253,7 @@ func TestCoalesceValues(t *testing.T) { tvals := &chart.Config{Raw: testCoalesceValuesYaml} - v, err := CoalesceValues(c, tvals, overrides) + v, err := CoalesceValues(c, tvals) j, _ := json.MarshalIndent(v, "", " ") t.Logf("Coalesced Values: %s", string(j)) @@ -207,7 +262,6 @@ func TestCoalesceValues(t *testing.T) { expect string }{ {"{{.top}}", "yup"}, - {"{{.override}}", "good"}, {"{{.name}}", "moby"}, {"{{.global.name}}", "Ishmael"}, {"{{.global.subject}}", "Queequeg"}, @@ -254,6 +308,9 @@ func TestCoalesceTables(t *testing.T) { "mast": true, }, } + + // What we expect is that anything in dst overrides anything in src, but that + // otherwise the values are coalesced. coalesceTables(dst, src) if dst["name"] != "Ishmael" { @@ -268,7 +325,7 @@ func TestCoalesceTables(t *testing.T) { t.Fatal("Address went away.") } - if addr["street"].(string) != "234 Spouter Inn Ct." { + if addr["street"].(string) != "123 Spouter Inn Ct." { t.Errorf("Unexpected address: %v", addr["street"]) } diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 46ec43e54..a61d5de37 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -54,18 +54,15 @@ func TestRender(t *testing.T) { } vals := &chart.Config{ - Raw: "outer: BAD\ninner: inn", - } - - overrides := map[string]interface{}{ - "outer": "spouter", - "global": map[string]interface{}{ - "callme": "Ishmael", - }, - } + Raw: ` +outer: spouter +inner: inn +global: + callme: Ishmael +`} e := New() - v, err := chartutil.CoalesceValues(c, vals, overrides) + v, err := chartutil.CoalesceValues(c, vals) if err != nil { t.Fatalf("Failed to coalesce values: %s", err) } @@ -271,7 +268,7 @@ global: when: to-day`, } - tmp, err := chartutil.CoalesceValues(outer, &injValues, map[string]interface{}{}) + tmp, err := chartutil.CoalesceValues(outer, &injValues) if err != nil { t.Fatalf("Failed to coalesce values: %s", err) }