From 490cef784cda22dbb3da1feafdd2b217c819b2fb Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Mon, 13 Jun 2016 13:11:39 -0600 Subject: [PATCH] fix(tiller): refactor template render to use chartutil. --- cmd/tiller/environment/environment.go | 3 +- cmd/tiller/environment/environment_test.go | 5 +- cmd/tiller/release_server.go | 7 +- pkg/chartutil/values.go | 14 ++- pkg/engine/engine.go | 108 ++------------------- pkg/engine/engine_test.go | 88 ++++------------- 6 files changed, 50 insertions(+), 175 deletions(-) diff --git a/cmd/tiller/environment/environment.go b/cmd/tiller/environment/environment.go index f0b5ff48c..8860f7d9b 100644 --- a/cmd/tiller/environment/environment.go +++ b/cmd/tiller/environment/environment.go @@ -9,6 +9,7 @@ package environment import ( "io" + "k8s.io/helm/pkg/chartutil" "k8s.io/helm/pkg/engine" "k8s.io/helm/pkg/kube" "k8s.io/helm/pkg/proto/hapi/chart" @@ -68,7 +69,7 @@ type Engine interface { // // It receives a chart, a config, and a map of overrides to the config. // Overrides are assumed to be passed from the system, not the user. - Render(*chart.Chart, *chart.Config, map[string]interface{}) (map[string]string, error) + Render(*chart.Chart, chartutil.Values) (map[string]string, error) } // ReleaseStorage represents a storage engine for a Release. diff --git a/cmd/tiller/environment/environment_test.go b/cmd/tiller/environment/environment_test.go index 1b842070f..c468b2198 100644 --- a/cmd/tiller/environment/environment_test.go +++ b/cmd/tiller/environment/environment_test.go @@ -5,6 +5,7 @@ import ( "io" "testing" + "k8s.io/helm/pkg/chartutil" "k8s.io/helm/pkg/proto/hapi/chart" "k8s.io/helm/pkg/proto/hapi/release" ) @@ -13,7 +14,7 @@ type mockEngine struct { out map[string]string } -func (e *mockEngine) Render(chrt *chart.Chart, v *chart.Config, o map[string]interface{}) (map[string]string, error) { +func (e *mockEngine) Render(chrt *chart.Chart, v chartutil.Values) (map[string]string, error) { return e.out, nil } @@ -80,7 +81,7 @@ func TestEngine(t *testing.T) { if engine, ok := env.EngineYard.Get("test"); !ok { t.Errorf("failed to get engine from EngineYard") - } else if out, err := engine.Render(&chart.Chart{}, &chart.Config{}, map[string]interface{}{}); err != nil { + } else if out, err := engine.Render(&chart.Chart{}, map[string]interface{}{}); err != nil { t.Errorf("unexpected template error: %s", err) } else if out["albatross"] != "test" { t.Errorf("expected 'test', got %q", out["albatross"]) diff --git a/cmd/tiller/release_server.go b/cmd/tiller/release_server.go index ce4f061ac..f879894a6 100644 --- a/cmd/tiller/release_server.go +++ b/cmd/tiller/release_server.go @@ -12,6 +12,7 @@ import ( ctx "golang.org/x/net/context" "k8s.io/helm/cmd/tiller/environment" + "k8s.io/helm/pkg/chartutil" "k8s.io/helm/pkg/proto/hapi/release" "k8s.io/helm/pkg/proto/hapi/services" "k8s.io/helm/pkg/storage" @@ -206,7 +207,11 @@ func (s *releaseServer) InstallRelease(c ctx.Context, req *services.InstallRelea // Render the templates // TODO: Fix based on whether chart has `engine: SOMETHING` set. - files, err := s.env.EngineYard.Default().Render(req.Chart, req.Values, overrides) + vals, err := chartutil.CoalesceValues(req.Chart, req.Values, overrides) + if err != nil { + return nil, err + } + files, err := s.env.EngineYard.Default().Render(req.Chart, vals) if err != nil { return nil, err } diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go index 09fd82a92..2e6a90043 100644 --- a/pkg/chartutil/values.go +++ b/pkg/chartutil/values.go @@ -87,6 +87,14 @@ 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 fillowing rules: +// +// - Values in a higher level chart always override values in a lower-level +// dependency chart +// - 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) { var cvals Values // Parse values if not nil. We merge these at the top level because @@ -152,15 +160,15 @@ func coalesceValues(c *chart.Chart, v map[string]interface{}) map[string]interfa // On error, we return just the overridden values. // FIXME: We should log this error. It indicates that the YAML data // did not parse. - log.Printf("error reading default values: %s", err) + log.Printf("error reading default values (%s): %s", c.Values.Raw, err) return v } for key, val := range nv { if _, ok := v[key]; !ok { v[key] = val - } else if dest, ok := v[key].(Values); ok { - src, ok := val.(Values) + } else if dest, ok := v[key].(map[string]interface{}); ok { + src, ok := val.(map[string]interface{}) if !ok { log.Printf("warning: skipped value for %s: Not a table.", key) continue diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 435d726ca..b456fdaec 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -3,7 +3,6 @@ package engine import ( "bytes" "fmt" - "log" "text/template" "github.com/Masterminds/sprig" @@ -45,41 +44,17 @@ func New() *Engine { // access to the values set for its parent. If chart "foo" includes chart "bar", // "bar" will not have access to the values for "foo". // +// Values should be prepared with something like `chartutils.ReadValues`. +// // Values are passed through the templates according to scope. If the top layer // chart includes the chart foo, which includes the chart bar, the values map // will be examined for a table called "foo". If "foo" is found in vals, // that section of the values will be passed into the "foo" chart. And if that // section contains a value named "bar", that value will be passed on to the // bar chart during render time. -// -// Values are coalesced together using the fillowing rules: -// -// - Values in a higher level chart always override values in a lower-level -// dependency chart -// - 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 (e *Engine) Render(chrt *chart.Chart, vals *chart.Config, overrides map[string]interface{}) (map[string]string, error) { - var cvals chartutil.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. - if vals != nil { - evals, err := chartutil.ReadValues([]byte(vals.Raw)) - if err != nil { - return map[string]string{}, 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) - } - +func (e *Engine) Render(chrt *chart.Chart, values chartutil.Values) (map[string]string, error) { // Render the charts - tmap := allTemplates(chrt, cvals) + tmap := allTemplates(chrt, values) return e.render(tmap) } @@ -138,20 +113,20 @@ func allTemplates(c *chart.Chart, vals chartutil.Values) map[string]renderable { // As it recurses, it also sets the values to be appropriate for the template // scope. func recAllTpls(c *chart.Chart, templates map[string]renderable, parentVals chartutil.Values, top bool) { - var pvals chartutil.Values + var cvals chartutil.Values if top { // If this is the top of the rendering tree, assume that parentVals // is already resolved to the authoritative values. - pvals = parentVals + cvals = parentVals } else if c.Metadata != nil && c.Metadata.Name != "" { // An error indicates that the table doesn't exist. So we leave it as // an empty map. tmp, err := parentVals.Table(c.Metadata.Name) if err == nil { - pvals = tmp + cvals = tmp } } - cvals := coalesceValues(c, pvals) + //log.Printf("racAllTpls values: %v", cvals) for _, child := range c.Dependencies { recAllTpls(child, templates, cvals, false) @@ -163,70 +138,3 @@ func recAllTpls(c *chart.Chart, templates map[string]renderable, parentVals char } } } - -// coalesceValues builds up a values map for a particular chart. -// -// Values in v will override the values in the chart. -func coalesceValues(c *chart.Chart, v chartutil.Values) chartutil.Values { - // If there are no values in the chart, we just return the given values - if c.Values == nil { - return v - } - - nv, err := chartutil.ReadValues([]byte(c.Values.Raw)) - if err != nil { - // On error, we return just the overridden values. - // FIXME: We should log this error. It indicates that the YAML data - // did not parse. - log.Printf("error reading default values: %s", err) - return v - } - - for k, val := range v { - // NOTE: We could block coalesce on cases where nv does not explicitly - // declare a value. But that forces the chart author to explicitly - // set a default for every template param. We want to preserve the - // possibility of "hidden" parameters. - if istable(val) { - if inmap, ok := nv[k]; ok && istable(inmap) { - coalesceTables(inmap.(map[string]interface{}), val.(map[string]interface{})) - } else if ok { - log.Printf("Cannot copy table into non-table value for %s (%v)", k, inmap) - } else { - // The parent table does not have a key entry for this item, - // so we can safely set it. This is necessary for nested charts. - log.Printf("Copying %s into map %v", k, nv) - nv[k] = val - } - } else { - nv[k] = val - } - } - return nv -} - -// coalesceTables merges a source map into a destination map. -func coalesceTables(dst, src map[string]interface{}) { - for key, val := range src { - if istable(val) { - if innerdst, ok := dst[key]; !ok { - dst[key] = val - } else if istable(innerdst) { - coalesceTables(innerdst.(map[string]interface{}), val.(map[string]interface{})) - } else { - log.Printf("Cannot overwrite table with non table for %s (%v)", key, val) - } - continue - } else if dv, ok := dst[key]; ok && istable(dv) { - log.Printf("Destination for %s is a table. Ignoring non-table value %v", key, val) - continue - } - dst[key] = val - } -} - -// istable is a special-purpose function to see if the present thing matches the definition of a YAML table. -func istable(v interface{}) bool { - _, ok := v.(map[string]interface{}) - return ok -} diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 095fb4b26..0753ed09c 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -44,7 +44,11 @@ func TestRender(t *testing.T) { } e := New() - out, err := e.Render(c, vals, overrides) + v, err := chartutil.CoalesceValues(c, vals, overrides) + if err != nil { + t.Fatalf("Failed to coalesce values: %s", err) + } + out, err := e.Render(c, v) if err != nil { t.Errorf("Failed to render templates: %s", err) } @@ -54,7 +58,7 @@ func TestRender(t *testing.T) { t.Errorf("Expected %q, got %q", expect, out["test1"]) } - if _, err := e.Render(c, &chart.Config{}, overrides); err != nil { + if _, err := e.Render(c, v); err != nil { t.Errorf("Unexpected error: %s", err) } } @@ -163,7 +167,7 @@ func TestRenderDependency(t *testing.T) { }, } - out, err := e.Render(ch, nil, map[string]interface{}{}) + out, err := e.Render(ch, map[string]interface{}{}) if err != nil { t.Fatalf("failed to render chart: %s", err) @@ -192,7 +196,7 @@ func TestRenderNestedValues(t *testing.T) { Templates: []*chart.Template{ {Name: deepestpath, Data: []byte(`And this same {{.what}} that smiles to-day`)}, }, - Values: &chart.Config{Raw: `what = "milkshake"`}, + Values: &chart.Config{Raw: `what: "milkshake"`}, } inner := &chart.Chart{ @@ -200,7 +204,7 @@ func TestRenderNestedValues(t *testing.T) { Templates: []*chart.Template{ {Name: innerpath, Data: []byte(`Old {{.who}} is still a-flyin'`)}, }, - Values: &chart.Config{Raw: `who = "Robert"`}, + Values: &chart.Config{Raw: `who: "Robert"`}, Dependencies: []*chart.Chart{deepest}, } @@ -212,13 +216,14 @@ func TestRenderNestedValues(t *testing.T) { Values: &chart.Config{ Raw: ` what: stinkweed +who: me herrick: - who: time`, + who: time`, }, Dependencies: []*chart.Chart{inner}, } - inject := chart.Config{ + injValues := chart.Config{ Raw: ` what: rosebuds herrick: @@ -226,7 +231,14 @@ herrick: what: flower`, } - out, err := e.Render(outer, &inject, map[string]interface{}{}) + inject, err := chartutil.CoalesceValues(outer, &injValues, map[string]interface{}{}) + if err != nil { + t.Fatalf("Failed to coalesce values: %s", err) + } + + t.Logf("Calculated values: %v", inject) + + out, err := e.Render(outer, inject) if err != nil { t.Fatalf("failed to render templates: %s", err) } @@ -243,63 +255,3 @@ herrick: t.Errorf("Unexpected deepest: %q", out[deepestpath]) } } - -func TestCoalesceTables(t *testing.T) { - dst := map[string]interface{}{ - "name": "Ishmael", - "address": map[string]interface{}{ - "street": "123 Spouter Inn Ct.", - "city": "Nantucket", - }, - "details": map[string]interface{}{ - "friends": []string{"Tashtego"}, - }, - "boat": "pequod", - } - src := map[string]interface{}{ - "occupation": "whaler", - "address": map[string]interface{}{ - "state": "MA", - "street": "234 Spouter Inn Ct.", - }, - "details": "empty", - "boat": map[string]interface{}{ - "mast": true, - }, - } - coalesceTables(dst, src) - - if dst["name"] != "Ishmael" { - t.Errorf("Unexpected name: %s", dst["name"]) - } - if dst["occupation"] != "whaler" { - t.Errorf("Unexpected occupation: %s", dst["occupation"]) - } - - addr, ok := dst["address"].(map[string]interface{}) - if !ok { - t.Fatal("Address went away.") - } - - if addr["street"].(string) != "234 Spouter Inn Ct." { - t.Errorf("Unexpected address: %v", addr["street"]) - } - - if addr["city"].(string) != "Nantucket" { - t.Errorf("Unexpected city: %v", addr["city"]) - } - - if addr["state"].(string) != "MA" { - t.Errorf("Unexpected state: %v", addr["state"]) - } - - if det, ok := dst["details"].(map[string]interface{}); !ok { - t.Fatalf("Details is the wrong type: %v", dst["details"]) - } else if _, ok := det["friends"]; !ok { - t.Error("Could not find your friends. Maybe you don't have any. :-(") - } - - if dst["boat"].(string) != "pequod" { - t.Errorf("Expected boat string, got %v", dst["boat"]) - } -}