From 33b1ede570aec8266eda547c0d8798c9e02f1064 Mon Sep 17 00:00:00 2001 From: Ian Howell Date: Mon, 8 Apr 2019 16:45:29 -0500 Subject: [PATCH 1/3] fix(pkg/engine): Clean up template error messages Signed-off-by: Ian Howell --- pkg/engine/engine.go | 43 ++++++++++++++++++++++++--------------- pkg/engine/engine_test.go | 27 ++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 25f1e6fad..89ea30bb2 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -17,6 +17,7 @@ limitations under the License. package engine import ( + "fmt" "path" "sort" "strings" @@ -150,51 +151,61 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) // higher-level (in file system) templates over deeply nested templates. keys := sortTemplates(tpls) - for _, fname := range keys { - r := tpls[fname] - if _, err := t.New(fname).Parse(r.tpl); err != nil { - return map[string]string{}, errors.Wrapf(err, "parse error in %q", fname) + for _, filename := range keys { + r := tpls[filename] + if _, err := t.New(filename).Parse(r.tpl); err != nil { + return map[string]string{}, parseTemplateError(err) } } // Adding the reference templates to the template context // so they can be referenced in the tpl function - for fname, r := range referenceTpls { - if t.Lookup(fname) == nil { - if _, err := t.New(fname).Parse(r.tpl); err != nil { - return map[string]string{}, errors.Wrapf(err, "parse error in %q", fname) + for filename, r := range referenceTpls { + if t.Lookup(filename) == nil { + if _, err := t.New(filename).Parse(r.tpl); err != nil { + return map[string]string{}, parseTemplateError(err) } } } rendered = make(map[string]string, len(keys)) - for _, file := range keys { + for _, filename := range keys { // Don't render partials. We don't care out the direct output of partials. // They are only included from other templates. - if strings.HasPrefix(path.Base(file), "_") { + if strings.HasPrefix(path.Base(filename), "_") { continue } // At render time, add information about the template that is being rendered. - vals := tpls[file].vals - vals["Template"] = chartutil.Values{"Name": file, "BasePath": tpls[file].basePath} + vals := tpls[filename].vals + vals["Template"] = chartutil.Values{"Name": filename, "BasePath": tpls[filename].basePath} var buf strings.Builder - if err := t.ExecuteTemplate(&buf, file, vals); err != nil { - return map[string]string{}, errors.Wrapf(err, "render error in %q", file) + if err := t.ExecuteTemplate(&buf, filename, vals); err != nil { + return map[string]string{}, parseTemplateError(err) } // Work around the issue where Go will emit "" even if Options(missing=zero) // is set. Since missing=error will never get here, we do not need to handle // the Strict case. f := &chart.File{ - Name: strings.ReplaceAll(file, "/templates", "/manifests"), + Name: strings.ReplaceAll(filename, "/templates", "/manifests"), Data: []byte(strings.ReplaceAll(buf.String(), "", "")), } - rendered[file] = string(f.Data) + rendered[filename] = string(f.Data) } return rendered, nil } +func parseTemplateError(err error) error { + tokens := strings.Split(err.Error(), ": ") + // The first token is "template" + // The second token is either "filename:lineno" or "filename:lineNo:columnNo" + location := tokens[1] + // The remaining tokens make up a stacktrace-like chain, ending with the relevant error + errMsg := tokens[len(tokens)-1] + return fmt.Errorf("%s (%s)", errMsg, string(location)) +} + func sortTemplates(tpls map[string]renderable) []string { keys := make([]string, len(tpls)) i := 0 diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index aebb2ad09..7c52077f5 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -187,6 +187,33 @@ func TestParallelRenderInternals(t *testing.T) { wg.Wait() } +func TestRenderErrors(t *testing.T) { + vals := chartutil.Values{"Values": map[string]interface{}{}} + + tplsMissingRequired := map[string]renderable{ + "missing_required": {tpl: `{{required "foo is required" .Values.foo}}`, vals: vals}, + } + _, err := new(Engine).render(tplsMissingRequired) + if err == nil { + t.Fatalf("Expected failures while rendering: %s", err) + } + expected := `foo is required (missing_required:1:2)` + if err.Error() != expected { + t.Errorf("Expected '%s', got %q", expected, err.Error()) + } + + tplsUndefinedFunction := map[string]renderable{ + "undefined_function": {tpl: `{{foo}}`, vals: vals}, + } + _, err = new(Engine).render(tplsUndefinedFunction) + if err == nil { + t.Fatalf("Expected failures while rendering: %s", err) + } + expected = `function "foo" not defined (undefined_function:1)` + if err.Error() != expected { + t.Errorf("Expected '%s', got %q", expected, err.Error()) + } +} func TestAllTemplates(t *testing.T) { ch1 := &chart.Chart{ Metadata: &chart.Metadata{Name: "ch1"}, From 92b86f6e74dc21a8dea00ee2fef1a345c0d99e8c Mon Sep 17 00:00:00 2001 From: Ian Howell Date: Tue, 9 Apr 2019 10:54:01 -0500 Subject: [PATCH 2/3] fix(pkg/engine): Catch non-templating errors when rendering templates Signed-off-by: Ian Howell --- pkg/engine/engine.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 89ea30bb2..937a3b4fd 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -154,7 +154,7 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) for _, filename := range keys { r := tpls[filename] if _, err := t.New(filename).Parse(r.tpl); err != nil { - return map[string]string{}, parseTemplateError(err) + return map[string]string{}, parseTemplateError(filename, err) } } @@ -163,7 +163,7 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) for filename, r := range referenceTpls { if t.Lookup(filename) == nil { if _, err := t.New(filename).Parse(r.tpl); err != nil { - return map[string]string{}, parseTemplateError(err) + return map[string]string{}, parseTemplateError(filename, err) } } } @@ -180,7 +180,7 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) vals["Template"] = chartutil.Values{"Name": filename, "BasePath": tpls[filename].basePath} var buf strings.Builder if err := t.ExecuteTemplate(&buf, filename, vals); err != nil { - return map[string]string{}, parseTemplateError(err) + return map[string]string{}, parseTemplateError(filename, err) } // Work around the issue where Go will emit "" even if Options(missing=zero) @@ -196,8 +196,12 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) return rendered, nil } -func parseTemplateError(err error) error { +func parseTemplateError(filename string, err error) error { tokens := strings.Split(err.Error(), ": ") + if len(tokens) == 1 { + // This might happen if a non-templating error occurs + return fmt.Errorf("render error in %s: %q", filename, err) + } // The first token is "template" // The second token is either "filename:lineno" or "filename:lineNo:columnNo" location := tokens[1] From 278594fb0fbd0a78ebb29fc286e6b60f6b9a7853 Mon Sep 17 00:00:00 2001 From: Ian Howell Date: Tue, 9 Apr 2019 12:34:13 -0500 Subject: [PATCH 3/3] fix(pkg/engine): Style changes on template errors Signed-off-by: Ian Howell --- pkg/engine/engine.go | 4 ++-- pkg/engine/engine_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 937a3b4fd..b1bc93040 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -200,14 +200,14 @@ func parseTemplateError(filename string, err error) error { tokens := strings.Split(err.Error(), ": ") if len(tokens) == 1 { // This might happen if a non-templating error occurs - return fmt.Errorf("render error in %s: %q", filename, err) + return fmt.Errorf("render error in (%s): %s", filename, err) } // The first token is "template" // The second token is either "filename:lineno" or "filename:lineNo:columnNo" location := tokens[1] // The remaining tokens make up a stacktrace-like chain, ending with the relevant error errMsg := tokens[len(tokens)-1] - return fmt.Errorf("%s (%s)", errMsg, string(location)) + return fmt.Errorf("render error at (%s): %s", string(location), errMsg) } func sortTemplates(tpls map[string]renderable) []string { diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 7c52077f5..9d708f193 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -197,7 +197,7 @@ func TestRenderErrors(t *testing.T) { if err == nil { t.Fatalf("Expected failures while rendering: %s", err) } - expected := `foo is required (missing_required:1:2)` + expected := `render error at (missing_required:1:2): foo is required` if err.Error() != expected { t.Errorf("Expected '%s', got %q", expected, err.Error()) } @@ -209,7 +209,7 @@ func TestRenderErrors(t *testing.T) { if err == nil { t.Fatalf("Expected failures while rendering: %s", err) } - expected = `function "foo" not defined (undefined_function:1)` + expected = `render error at (undefined_function:1): function "foo" not defined` if err.Error() != expected { t.Errorf("Expected '%s', got %q", expected, err.Error()) }