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

@@ -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
 	}

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
 		}

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.
+		cvals = coalesce(chrt, evals)
-		// 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 = 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
+		}
 	}
 	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
+	top["Values"] = vals
-	return overrides, nil
+	return top, nil
 }
 
 // istable is a special-purpose function to see if the present thing matches the definition of a YAML table.

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"])
 	}
 

pkg/engine/engine_test.go

@@ -54,18 +54,15 @@ func TestRender(t *testing.T) {
 	}
 
 	vals := &chart.Config{
-		Raw: "outer: BAD\ninner: inn",
+		Raw: `
-	}
+outer: spouter
-
+inner: inn
-	overrides := map[string]interface{}{
+global:
-		"outer": "spouter",
+  callme: Ishmael
-		"global": map[string]interface{}{
+`}
-			"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)
 	}