From 14f902c2455ee29436a4d23532db5ee234f3fc7e Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 2 Dec 2024 21:52:41 -0500 Subject: [PATCH 01/35] wip: draft at making cleaner stacktraces Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 50 ++++++++++++++++++++++++++++++++++++++- pkg/engine/engine_test.go | 21 ++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 839ad4a31..00b3d917a 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -330,6 +330,11 @@ func cleanupParseError(filename string, err error) error { return fmt.Errorf("parse error at (%s): %s", string(location), errMsg) } +type TraceableError struct { + location string + message string +} + func cleanupExecError(filename string, err error) error { if _, isExecError := err.(template.ExecError); !isExecError { return err @@ -349,8 +354,51 @@ func cleanupExecError(filename string, err error) error { if len(parts) >= 2 { return fmt.Errorf("execution error at (%s): %s", string(location), parts[1]) } + current := err + fileLocations := []TraceableError{} + for { + if current == nil { + break + } + tokens = strings.SplitN(current.Error(), ": ", 3) + location = tokens[1] + traceable := TraceableError{ + location: location, + message: current.Error(), + } + fileLocations = append(fileLocations, traceable) + current = errors.Unwrap(current) + } + + prevMessage := "" + for i := len(fileLocations) - 1; i >= 0; i-- { + currentMsg := fileLocations[i].message + if i == len(fileLocations)-1 { + prevMessage = currentMsg + continue + } + + if strings.Contains(currentMsg, prevMessage) { + fileLocations[i].message = strings.ReplaceAll(fileLocations[i].message, prevMessage, "") + } + prevMessage = currentMsg + } + + for i := len(fileLocations) - 1; i >= 0; i-- { + if strings.Contains(fileLocations[i].message, fileLocations[i].location) { + fileLocations[i].message = strings.ReplaceAll(fileLocations[i].message, fileLocations[i].location, "") + } + } + + finalErrorString := "" + for _, i := range fileLocations { + if i.message == "" { + continue + } + finalErrorString = finalErrorString + "\n" + i.location + " " + i.message + } - return err + return fmt.Errorf("%s\n\n\n\nError: %s", finalErrorString, err.Error()) } func sortTemplates(tpls map[string]renderable) []string { diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 68e0158fa..8445abf83 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -18,6 +18,8 @@ package engine import ( "fmt" + "github.com/stretchr/testify/assert" + "helm.sh/helm/v4/pkg/chart/v2/loader" "path" "strings" "sync" @@ -1301,6 +1303,25 @@ func TestRenderTplMissingKeyString(t *testing.T) { } } +func TestSometimesJesseJustBe(t *testing.T) { + c, _ := loader.Load("/home/jesse/code/camunda-platform-helm/charts/camunda-platform-8.5") + + v, _ := chartutil.ReadValuesFile("/home/jesse/code/helm/values.yaml") + val, _ := chartutil.CoalesceValues(c, v) + vals := map[string]interface{}{ + "Values": val.AsMap(), + } + out, err := Render(c, vals) + + if err != nil { + t.Errorf("Failed to render templates: %s", err) + } + assert.NotNil(t, out) + data := strings.TrimSpace(out["jesse-subchart-values-hacktest/charts/keycloak/templates/ingress.yaml"]) + fmt.Println(data) + assert.NotEmpty(t, data) +} + func TestRenderCustomTemplateFuncs(t *testing.T) { // Create a chart with two templates that use custom functions c := &chart.Chart{ From cc477e9f7919ad6a6962e5f1076acde9db0e37db Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Tue, 3 Dec 2024 18:32:56 -0500 Subject: [PATCH 02/35] fix: adjust test to not require external chart Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 2 +- pkg/engine/engine_test.go | 28 +++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 00b3d917a..65e633f89 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -398,7 +398,7 @@ func cleanupExecError(filename string, err error) error { finalErrorString = finalErrorString + "\n" + i.location + " " + i.message } - return fmt.Errorf("%s\n\n\n\nError: %s", finalErrorString, err.Error()) + return fmt.Errorf("NEW ERROR FORMAT: \n%s\n\n\nORIGINAL ERROR:\n%s", finalErrorString, err.Error()) } func sortTemplates(tpls map[string]renderable) []string { diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 8445abf83..fc2a1a3f3 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -19,7 +19,6 @@ package engine import ( "fmt" "github.com/stretchr/testify/assert" - "helm.sh/helm/v4/pkg/chart/v2/loader" "path" "strings" "sync" @@ -1303,23 +1302,34 @@ func TestRenderTplMissingKeyString(t *testing.T) { } } -func TestSometimesJesseJustBe(t *testing.T) { - c, _ := loader.Load("/home/jesse/code/camunda-platform-helm/charts/camunda-platform-8.5") +func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "NestedHelperFunctions"}, + Templates: []*chart.File{ + {Name: "templates/svc.yaml", Data: []byte( + `name: {{ include "nested_helper.name" . }}`, + )}, + {Name: "templates/_helpers_1.tpl", Data: []byte( + `{{- define "nested_helper.name" -}}{{- include "common.names.get_name" . -}}{{- end -}}`, + )}, + {Name: "charts/common/templates/_helpers_2.tpl", Data: []byte( + `{{- define "common.names.get_name" -}}{{- .Release.Name | trunc 63 | trimSuffix "-" -}}{{- end -}}`, + )}, + }, + } + + v := chartutil.Values{} - v, _ := chartutil.ReadValuesFile("/home/jesse/code/helm/values.yaml") val, _ := chartutil.CoalesceValues(c, v) vals := map[string]interface{}{ "Values": val.AsMap(), } - out, err := Render(c, vals) + _, err := Render(c, vals) + assert.NotNil(t, err) if err != nil { t.Errorf("Failed to render templates: %s", err) } - assert.NotNil(t, out) - data := strings.TrimSpace(out["jesse-subchart-values-hacktest/charts/keycloak/templates/ingress.yaml"]) - fmt.Println(data) - assert.NotEmpty(t, data) } func TestRenderCustomTemplateFuncs(t *testing.T) { From 6cd0c0082a4f0c996adc7d71328b5d0d8953f1b8 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 14:44:37 -0500 Subject: [PATCH 03/35] fix: split up the multiline errors to be more vertical Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 65e633f89..0336801c6 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -331,8 +331,9 @@ func cleanupParseError(filename string, err error) error { } type TraceableError struct { - location string - message string + location string + message string + executedFunction string } func cleanupExecError(filename string, err error) error { @@ -390,12 +391,35 @@ func cleanupExecError(filename string, err error) error { } } + for i := len(fileLocations) - 1; i >= 0; i-- { + if strings.Contains(fileLocations[i].message, "template:") { + fileLocations[i].message = strings.TrimSpace(strings.ReplaceAll(fileLocations[i].message, "template:", "")) + } + if strings.HasPrefix(fileLocations[i].message, ": ") { + fileLocations[i].message = strings.TrimSpace(strings.TrimPrefix(fileLocations[i].message, ": ")) + } + } + + for i := len(fileLocations) - 1; i >= 0; i-- { + if fileLocations[i].message == "" { + continue + } + executionLocationRegex, regexFindErr := regexp.Compile(`executing "[^\"]*" at <[^\<\>]*>:?\s*`) + if regexFindErr != nil { + continue + } + byteArrayMsg := []byte(fileLocations[i].message) + executionLocations := executionLocationRegex.FindAll(byteArrayMsg, -1) + fileLocations[i].executedFunction = string(executionLocations[0]) + fileLocations[i].message = strings.ReplaceAll(fileLocations[i].message, fileLocations[i].executedFunction, "") + } + finalErrorString := "" for _, i := range fileLocations { if i.message == "" { continue } - finalErrorString = finalErrorString + "\n" + i.location + " " + i.message + finalErrorString = finalErrorString + "\n" + i.location + "\n " + i.executedFunction + "\n " + i.message } return fmt.Errorf("NEW ERROR FORMAT: \n%s\n\n\nORIGINAL ERROR:\n%s", finalErrorString, err.Error()) From 87f9e2dc45532bb94421c9c817e12c5b37c789f8 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 15:35:39 -0500 Subject: [PATCH 04/35] style: create String function for TraceableError representing one chunk of a stacktrace Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 0336801c6..5234ce293 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -336,6 +336,10 @@ type TraceableError struct { executedFunction string } +func (t TraceableError) String() string { + return t.location + "\n " + t.executedFunction + "\n " + t.message + "\n" +} + func cleanupExecError(filename string, err error) error { if _, isExecError := err.(template.ExecError); !isExecError { return err @@ -419,7 +423,7 @@ func cleanupExecError(filename string, err error) error { if i.message == "" { continue } - finalErrorString = finalErrorString + "\n" + i.location + "\n " + i.executedFunction + "\n " + i.message + finalErrorString = finalErrorString + i.String() } return fmt.Errorf("NEW ERROR FORMAT: \n%s\n\n\nORIGINAL ERROR:\n%s", finalErrorString, err.Error()) From a56daca82bb4b741bf59a6c1e2e7c8772d520859 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 15:53:20 -0500 Subject: [PATCH 05/35] style: use interface functions instead of inline logic Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 58 +++++++++++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 5234ce293..273560212 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -340,6 +340,35 @@ func (t TraceableError) String() string { return t.location + "\n " + t.executedFunction + "\n " + t.message + "\n" } +func (t TraceableError) ExtractExecutedFunction() (TraceableError, error) { + executionLocationRegex, regexFindErr := regexp.Compile(`executing "[^\"]*" at <[^\<\>]*>:?\s*`) + if regexFindErr != nil { + return t, regexFindErr + } + byteArrayMsg := []byte(t.message) + executionLocations := executionLocationRegex.FindAll(byteArrayMsg, -1) + t.executedFunction = string(executionLocations[0]) + t.message = strings.ReplaceAll(t.message, t.executedFunction, "") + return t, nil +} + +func (t TraceableError) FilterLocation() TraceableError { + if strings.Contains(t.message, t.location) { + t.message = strings.ReplaceAll(t.message, t.location, "") + } + return t +} + +func (t TraceableError) FilterUnnecessaryWords() TraceableError { + if strings.Contains(t.message, "template:") { + t.message = strings.TrimSpace(strings.ReplaceAll(t.message, "template:", "")) + } + if strings.HasPrefix(t.message, ": ") { + t.message = strings.TrimSpace(strings.TrimPrefix(t.message, ": ")) + } + return t +} + func cleanupExecError(filename string, err error) error { if _, isExecError := err.(template.ExecError); !isExecError { return err @@ -389,33 +418,24 @@ func cleanupExecError(filename string, err error) error { prevMessage = currentMsg } - for i := len(fileLocations) - 1; i >= 0; i-- { - if strings.Contains(fileLocations[i].message, fileLocations[i].location) { - fileLocations[i].message = strings.ReplaceAll(fileLocations[i].message, fileLocations[i].location, "") - } + for i, fileLocation := range fileLocations { + t := fileLocation.FilterLocation() + fileLocations[i] = t } - for i := len(fileLocations) - 1; i >= 0; i-- { - if strings.Contains(fileLocations[i].message, "template:") { - fileLocations[i].message = strings.TrimSpace(strings.ReplaceAll(fileLocations[i].message, "template:", "")) - } - if strings.HasPrefix(fileLocations[i].message, ": ") { - fileLocations[i].message = strings.TrimSpace(strings.TrimPrefix(fileLocations[i].message, ": ")) - } + for i, fileLocation := range fileLocations { + fileLocations[i] = fileLocation.FilterUnnecessaryWords() } - for i := len(fileLocations) - 1; i >= 0; i-- { - if fileLocations[i].message == "" { + for i, fileLocation := range fileLocations { + if fileLocation.message == "" { continue } - executionLocationRegex, regexFindErr := regexp.Compile(`executing "[^\"]*" at <[^\<\>]*>:?\s*`) - if regexFindErr != nil { + t, extractionErr := fileLocation.ExtractExecutedFunction() + if extractionErr != nil { continue } - byteArrayMsg := []byte(fileLocations[i].message) - executionLocations := executionLocationRegex.FindAll(byteArrayMsg, -1) - fileLocations[i].executedFunction = string(executionLocations[0]) - fileLocations[i].message = strings.ReplaceAll(fileLocations[i].message, fileLocations[i].executedFunction, "") + fileLocations[i] = t } finalErrorString := "" From 1357db4db133a486d69f00072bf23754765e9fbf Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 15:54:13 -0500 Subject: [PATCH 06/35] style: removed variable only used once Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 273560212..156aafcf0 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -419,8 +419,7 @@ func cleanupExecError(filename string, err error) error { } for i, fileLocation := range fileLocations { - t := fileLocation.FilterLocation() - fileLocations[i] = t + fileLocations[i] = fileLocation.FilterLocation() } for i, fileLocation := range fileLocations { From f09bbb8ab8b0a034bfc4cfbca35c395ed332ba2d Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 16:19:16 -0500 Subject: [PATCH 07/35] style: consolidate for loops Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 156aafcf0..6a9b49a3d 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -419,14 +419,7 @@ func cleanupExecError(filename string, err error) error { } for i, fileLocation := range fileLocations { - fileLocations[i] = fileLocation.FilterLocation() - } - - for i, fileLocation := range fileLocations { - fileLocations[i] = fileLocation.FilterUnnecessaryWords() - } - - for i, fileLocation := range fileLocations { + fileLocations[i] = fileLocation.FilterLocation().FilterUnnecessaryWords() if fileLocation.message == "" { continue } From 2e3f6dce28b1c204bf99d6ba660365a0ca23adfe Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 16:25:18 -0500 Subject: [PATCH 08/35] fix: save to fileLocation rather than fileLocations[i] which gets overwritten Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 6a9b49a3d..a533e3241 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -419,7 +419,7 @@ func cleanupExecError(filename string, err error) error { } for i, fileLocation := range fileLocations { - fileLocations[i] = fileLocation.FilterLocation().FilterUnnecessaryWords() + fileLocation = fileLocation.FilterLocation().FilterUnnecessaryWords() if fileLocation.message == "" { continue } From c48147098534e935ea5f9725fbfd04f39431791b Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 16:27:31 -0500 Subject: [PATCH 09/35] style: renamed i variable for consistency Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index a533e3241..21d3fd2c8 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -431,11 +431,11 @@ func cleanupExecError(filename string, err error) error { } finalErrorString := "" - for _, i := range fileLocations { - if i.message == "" { + for _, fileLocation := range fileLocations { + if fileLocation.message == "" { continue } - finalErrorString = finalErrorString + i.String() + finalErrorString = finalErrorString + fileLocation.String() } return fmt.Errorf("NEW ERROR FORMAT: \n%s\n\n\nORIGINAL ERROR:\n%s", finalErrorString, err.Error()) From d8bec4e30f6f2aa068d0b90e68266d6cba7c8253 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 23 Dec 2024 16:33:03 -0500 Subject: [PATCH 10/35] fix: remove comparison from old error message to new Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 21d3fd2c8..4c7021872 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -437,8 +437,12 @@ func cleanupExecError(filename string, err error) error { } finalErrorString = finalErrorString + fileLocation.String() } + if strings.TrimSpace(finalErrorString) == "" { + // Fallback to original error message if nothing was extracted + return fmt.Errorf("%s", err.Error()) + } - return fmt.Errorf("NEW ERROR FORMAT: \n%s\n\n\nORIGINAL ERROR:\n%s", finalErrorString, err.Error()) + return fmt.Errorf("%s", finalErrorString) } func sortTemplates(tpls map[string]renderable) []string { From 383a758aecd00e6c1bd7043f0b6b9c8151d850eb Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Tue, 31 Dec 2024 18:47:09 -0500 Subject: [PATCH 11/35] fix: add quality checks to ensure formatted error doesnt remove important info Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 4c7021872..9d81a90db 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -369,6 +369,33 @@ func (t TraceableError) FilterUnnecessaryWords() TraceableError { return t } +// In the process of formatting the error, we want to ensure that the formatted version of the error +// is not losing any necessary information. This function will tokenize and compare the two strings +// and if the formatted error doesn't meet the threshold, it will fallback to the originalErr +func determineIfFormattedErrorIsAcceptable(formattedErr error, originalErr error) error { + formattedErrTokens := strings.Fields(formattedErr.Error()) + originalErrTokens := strings.Fields(originalErr.Error()) + + tokenSet := make(map[string]struct{}) + for _, token := range originalErrTokens { + tokenSet[token] = struct{}{} + } + + matchCount := 0 + for _, token := range formattedErrTokens { + if _, exists := tokenSet[token]; exists { + matchCount++ + } + } + + equivalenceRating := (float64(matchCount) / float64(len(formattedErrTokens))) * 100 + fmt.Printf("Rating: %f\n", equivalenceRating) + if equivalenceRating >= 80 { + return formattedErr + } + return originalErr +} + func cleanupExecError(filename string, err error) error { if _, isExecError := err.(template.ExecError); !isExecError { return err @@ -442,7 +469,7 @@ func cleanupExecError(filename string, err error) error { return fmt.Errorf("%s", err.Error()) } - return fmt.Errorf("%s", finalErrorString) + return determineIfFormattedErrorIsAcceptable(fmt.Errorf("%s", finalErrorString), err) } func sortTemplates(tpls map[string]renderable) []string { From 6bb836374b1b27841f2ed1db93028e7502cb8148 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Tue, 31 Dec 2024 18:54:34 -0500 Subject: [PATCH 12/35] test: adjusted to make it more meaningful and to pass Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 1 - pkg/engine/engine_test.go | 15 ++++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 9d81a90db..c8e50eed5 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -389,7 +389,6 @@ func determineIfFormattedErrorIsAcceptable(formattedErr error, originalErr error } equivalenceRating := (float64(matchCount) / float64(len(formattedErrTokens))) * 100 - fmt.Printf("Rating: %f\n", equivalenceRating) if equivalenceRating >= 80 { return formattedErr } diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index fc2a1a3f3..55fb0087c 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -1318,6 +1318,17 @@ func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { }, } + expectedErrorMessage := `NestedHelperFunctions/templates/svc.yaml:1:9 + executing "NestedHelperFunctions/templates/svc.yaml" at : + error calling include: +NestedHelperFunctions/templates/_helpers_1.tpl:1:39 + executing "nested_helper.name" at : + error calling include: +NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:50 + executing "common.names.get_name" at <.Release.Name>: + nil pointer evaluating interface {}.Name +` + v := chartutil.Values{} val, _ := chartutil.CoalesceValues(c, v) @@ -1327,9 +1338,7 @@ func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { _, err := Render(c, vals) assert.NotNil(t, err) - if err != nil { - t.Errorf("Failed to render templates: %s", err) - } + assert.Equal(t, expectedErrorMessage, err.Error()) } func TestRenderCustomTemplateFuncs(t *testing.T) { From 487f72b822655185c279e87a4bef51c7b51792c6 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Wed, 1 Jan 2025 12:26:39 -0500 Subject: [PATCH 13/35] fix: added protection against while-true condition in unbounded for loop Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index c8e50eed5..e06c18f34 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -392,7 +392,7 @@ func determineIfFormattedErrorIsAcceptable(formattedErr error, originalErr error if equivalenceRating >= 80 { return formattedErr } - return originalErr + return fmt.Errorf("%s", originalErr.Error()) } func cleanupExecError(filename string, err error) error { @@ -416,7 +416,8 @@ func cleanupExecError(filename string, err error) error { } current := err fileLocations := []TraceableError{} - for { + maxIterations := 100 + for i := 0; i < maxIterations && current != nil; i++ { if current == nil { break } @@ -429,6 +430,9 @@ func cleanupExecError(filename string, err error) error { fileLocations = append(fileLocations, traceable) current = errors.Unwrap(current) } + if current != nil { + return fmt.Errorf("%s", err.Error()) + } prevMessage := "" for i := len(fileLocations) - 1; i >= 0; i-- { From edf0f7be592c3909ece2735f93278ce9d859b813 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 25 Jan 2025 09:04:21 -0500 Subject: [PATCH 14/35] test: adjust formatting for error in test Signed-off-by: Jesse Simpson --- pkg/action/install_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index e39674c80..6a028bd85 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -446,7 +446,9 @@ func TestInstallReleaseIncorrectTemplate_DryRun(t *testing.T) { instAction.DryRun = true vals := map[string]interface{}{} _, err := instAction.Run(buildChart(withSampleIncludingIncorrectTemplates()), vals) - expectedErr := "\"hello/templates/incorrect\" at <.Values.bad.doh>: nil pointer evaluating interface {}.doh" + expectedErr := `hello/templates/incorrect:1:10 + executing "hello/templates/incorrect" at <.Values.bad.doh>: + nil pointer evaluating interface {}.doh` if err == nil { t.Fatalf("Install should fail containing error: %s", expectedErr) } From 80d7a1b33f556c03bbf3b39fea488ca34731a2d2 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 25 Jan 2025 09:26:02 -0500 Subject: [PATCH 15/35] style: make format Signed-off-by: Jesse Simpson --- pkg/engine/engine_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 55fb0087c..79560bc75 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -18,13 +18,14 @@ package engine import ( "fmt" - "github.com/stretchr/testify/assert" "path" "strings" "sync" "testing" "text/template" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" From 98da3e28b666b04664a787e515d2a3adc1fb0289 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 25 Jan 2025 10:06:06 -0500 Subject: [PATCH 16/35] fix: add some index checking and fixed a test that relied on type-checking Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 10 +++++++++- pkg/engine/engine_test.go | 14 ++++---------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index e06c18f34..0c7d72dba 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -347,6 +347,9 @@ func (t TraceableError) ExtractExecutedFunction() (TraceableError, error) { } byteArrayMsg := []byte(t.message) executionLocations := executionLocationRegex.FindAll(byteArrayMsg, -1) + if len(executionLocations) == 0 { + return t, nil + } t.executedFunction = string(executionLocations[0]) t.message = strings.ReplaceAll(t.message, t.executedFunction, "") return t, nil @@ -422,7 +425,12 @@ func cleanupExecError(filename string, err error) error { break } tokens = strings.SplitN(current.Error(), ": ", 3) - location = tokens[1] + if len(tokens) == 1 { + // For cases where the error message doesn't contain a colon + location = tokens[0] + } else { + location = tokens[1] + } traceable := TraceableError{ location: location, message: current.Error(), diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 79560bc75..a34082e5f 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -22,7 +22,6 @@ import ( "strings" "sync" "testing" - "text/template" "github.com/stretchr/testify/assert" @@ -1291,16 +1290,11 @@ func TestRenderTplMissingKeyString(t *testing.T) { t.Errorf("Expected error, got %v", out) return } - switch err.(type) { - case (template.ExecError): - errTxt := fmt.Sprint(err) - if !strings.Contains(errTxt, "noSuchKey") { - t.Errorf("Expected error to contain 'noSuchKey', got %s", errTxt) - } - default: - // Some unexpected error. - t.Fatal(err) + errTxt := fmt.Sprint(err) + if !strings.Contains(errTxt, "noSuchKey") { + t.Errorf("Expected error to contain 'noSuchKey', got %s", errTxt) } + } func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { From daf4c348791a81607dc5ced88fff3b18ede847a4 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 22 Feb 2025 09:46:21 -0500 Subject: [PATCH 17/35] fix: use originalErr instead of formatting original error Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 0c7d72dba..d70e9fdad 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -395,7 +395,7 @@ func determineIfFormattedErrorIsAcceptable(formattedErr error, originalErr error if equivalenceRating >= 80 { return formattedErr } - return fmt.Errorf("%s", originalErr.Error()) + return originalErr } func cleanupExecError(filename string, err error) error { From 13b232e061f0d33d365beb9dc28ca72656051552 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 22 Feb 2025 11:58:11 -0500 Subject: [PATCH 18/35] refactor: make use of regexs for err parsing Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 132 ++++++++++++++------------------------ pkg/engine/engine_test.go | 6 +- 2 files changed, 50 insertions(+), 88 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index d70e9fdad..48020a521 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -339,67 +339,22 @@ type TraceableError struct { func (t TraceableError) String() string { return t.location + "\n " + t.executedFunction + "\n " + t.message + "\n" } - -func (t TraceableError) ExtractExecutedFunction() (TraceableError, error) { - executionLocationRegex, regexFindErr := regexp.Compile(`executing "[^\"]*" at <[^\<\>]*>:?\s*`) - if regexFindErr != nil { - return t, regexFindErr - } - byteArrayMsg := []byte(t.message) - executionLocations := executionLocationRegex.FindAll(byteArrayMsg, -1) - if len(executionLocations) == 0 { - return t, nil - } - t.executedFunction = string(executionLocations[0]) - t.message = strings.ReplaceAll(t.message, t.executedFunction, "") - return t, nil -} - -func (t TraceableError) FilterLocation() TraceableError { - if strings.Contains(t.message, t.location) { - t.message = strings.ReplaceAll(t.message, t.location, "") - } - return t -} - -func (t TraceableError) FilterUnnecessaryWords() TraceableError { - if strings.Contains(t.message, "template:") { - t.message = strings.TrimSpace(strings.ReplaceAll(t.message, "template:", "")) - } - if strings.HasPrefix(t.message, ": ") { - t.message = strings.TrimSpace(strings.TrimPrefix(t.message, ": ")) - } - return t -} - -// In the process of formatting the error, we want to ensure that the formatted version of the error -// is not losing any necessary information. This function will tokenize and compare the two strings -// and if the formatted error doesn't meet the threshold, it will fallback to the originalErr -func determineIfFormattedErrorIsAcceptable(formattedErr error, originalErr error) error { - formattedErrTokens := strings.Fields(formattedErr.Error()) - originalErrTokens := strings.Fields(originalErr.Error()) - - tokenSet := make(map[string]struct{}) - for _, token := range originalErrTokens { - tokenSet[token] = struct{}{} +func cleanupExecError(filename string, err error) error { + if _, isExecError := err.(template.ExecError); !isExecError { + return err } - matchCount := 0 - for _, token := range formattedErrTokens { - if _, exists := tokenSet[token]; exists { - matchCount++ - } - } + // taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=138 + // > "template: %s: %s" + // taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=141 + // > "template: %s: executing %q at <%s>: %s" - equivalenceRating := (float64(matchCount) / float64(len(formattedErrTokens))) * 100 - if equivalenceRating >= 80 { - return formattedErr + execErrFmt, compileErr := regexp.Compile(`^template: (?P(?U).+): executing (?P(?U).+) at (?P(?U).+): (?P(?U).+)(?P( template:.*)?)$`) + if compileErr != nil { + return err } - return originalErr -} - -func cleanupExecError(filename string, err error) error { - if _, isExecError := err.(template.ExecError); !isExecError { + execErrFmtWithoutTemplate, compileErr := regexp.Compile(`^template: (?P(?U).+): (?P.*)(?P( template:.*)?)$`) + if compileErr != nil { return err } @@ -424,16 +379,35 @@ func cleanupExecError(filename string, err error) error { if current == nil { break } - tokens = strings.SplitN(current.Error(), ": ", 3) - if len(tokens) == 1 { - // For cases where the error message doesn't contain a colon - location = tokens[0] + + var traceable TraceableError + if execErrFmt.MatchString(current.Error()) { + matches := execErrFmt.FindStringSubmatch(current.Error()) + templateIndex := execErrFmt.SubexpIndex("templateName") + templateName := matches[templateIndex] + functionNameIndex := execErrFmt.SubexpIndex("functionName") + functionName := matches[functionNameIndex] + locationNameIndex := execErrFmt.SubexpIndex("location") + locationName := matches[locationNameIndex] + errMsgIndex := execErrFmt.SubexpIndex("errMsg") + errMsg := matches[errMsgIndex] + traceable = TraceableError{ + location: templateName, + message: errMsg, + executedFunction: "executing " + functionName + " at " + locationName + ":", + } + } else if execErrFmtWithoutTemplate.MatchString(current.Error()) { + matches := execErrFmt.FindStringSubmatch(current.Error()) + templateIndex := execErrFmt.SubexpIndex("templateName") + templateName := matches[templateIndex] + errMsgIndex := execErrFmt.SubexpIndex("errMsg") + errMsg := matches[errMsgIndex] + traceable = TraceableError{ + location: templateName, + message: errMsg, + } } else { - location = tokens[1] - } - traceable := TraceableError{ - location: location, - message: current.Error(), + return err } fileLocations = append(fileLocations, traceable) current = errors.Unwrap(current) @@ -442,30 +416,18 @@ func cleanupExecError(filename string, err error) error { return fmt.Errorf("%s", err.Error()) } - prevMessage := "" + var prev TraceableError for i := len(fileLocations) - 1; i >= 0; i-- { - currentMsg := fileLocations[i].message + current := fileLocations[i] if i == len(fileLocations)-1 { - prevMessage = currentMsg + prev = current continue } - if strings.Contains(currentMsg, prevMessage) { - fileLocations[i].message = strings.ReplaceAll(fileLocations[i].message, prevMessage, "") - } - prevMessage = currentMsg - } - - for i, fileLocation := range fileLocations { - fileLocation = fileLocation.FilterLocation().FilterUnnecessaryWords() - if fileLocation.message == "" { - continue - } - t, extractionErr := fileLocation.ExtractExecutedFunction() - if extractionErr != nil { - continue + if current.message == prev.message && current.location == prev.location && current.executedFunction == prev.executedFunction { + fileLocations[i].message = "" } - fileLocations[i] = t + prev = current } finalErrorString := "" @@ -480,7 +442,7 @@ func cleanupExecError(filename string, err error) error { return fmt.Errorf("%s", err.Error()) } - return determineIfFormattedErrorIsAcceptable(fmt.Errorf("%s", finalErrorString), err) + return fmt.Errorf("%s", finalErrorString) } func sortTemplates(tpls map[string]renderable) []string { diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index a34082e5f..58d8588ab 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -1314,13 +1314,13 @@ func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { } expectedErrorMessage := `NestedHelperFunctions/templates/svc.yaml:1:9 - executing "NestedHelperFunctions/templates/svc.yaml" at : + executing "NestedHelperFunctions/templates/svc.yaml" at : error calling include: NestedHelperFunctions/templates/_helpers_1.tpl:1:39 - executing "nested_helper.name" at : + executing "nested_helper.name" at : error calling include: NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:50 - executing "common.names.get_name" at <.Release.Name>: + executing "common.names.get_name" at <.Release.Name>: nil pointer evaluating interface {}.Name ` From 0a7dd4b269da6b8f739c7189b06227e16c3dafe7 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 22 Feb 2025 12:15:49 -0500 Subject: [PATCH 19/35] test: adjust failing test on extra whitespace Signed-off-by: Jesse Simpson --- pkg/action/install_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 6a028bd85..61cc2946f 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -447,7 +447,7 @@ func TestInstallReleaseIncorrectTemplate_DryRun(t *testing.T) { vals := map[string]interface{}{} _, err := instAction.Run(buildChart(withSampleIncludingIncorrectTemplates()), vals) expectedErr := `hello/templates/incorrect:1:10 - executing "hello/templates/incorrect" at <.Values.bad.doh>: + executing "hello/templates/incorrect" at <.Values.bad.doh>: nil pointer evaluating interface {}.doh` if err == nil { t.Fatalf("Install should fail containing error: %s", expectedErr) From deea4a0d0e91f7d25761a07713fbcb362cc46f49 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 22 Feb 2025 12:34:05 -0500 Subject: [PATCH 20/35] refactor: remove more unnecessary format calls Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 48020a521..74cc899ef 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -413,7 +413,7 @@ func cleanupExecError(filename string, err error) error { current = errors.Unwrap(current) } if current != nil { - return fmt.Errorf("%s", err.Error()) + return err } var prev TraceableError @@ -439,7 +439,7 @@ func cleanupExecError(filename string, err error) error { } if strings.TrimSpace(finalErrorString) == "" { // Fallback to original error message if nothing was extracted - return fmt.Errorf("%s", err.Error()) + return err } return fmt.Errorf("%s", finalErrorString) From 3cc4cb60ba3316e25c630d263a0aad9225441af0 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 22 Feb 2025 13:10:07 -0500 Subject: [PATCH 21/35] refactor: prevent duplicates being inserted rather than post-filtering Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 74cc899ef..d5c507eab 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -379,6 +379,9 @@ func cleanupExecError(filename string, err error) error { if current == nil { break } + if i == maxIterations-1 { + return err + } var traceable TraceableError if execErrFmt.MatchString(current.Error()) { @@ -409,34 +412,24 @@ func cleanupExecError(filename string, err error) error { } else { return err } + if len(fileLocations) > 0 { + lastErr := fileLocations[len(fileLocations)-1] + if lastErr.message == traceable.message && + lastErr.location == traceable.location && + lastErr.executedFunction == traceable.executedFunction { + current = errors.Unwrap(current) + continue + } + } fileLocations = append(fileLocations, traceable) current = errors.Unwrap(current) } - if current != nil { - return err - } - - var prev TraceableError - for i := len(fileLocations) - 1; i >= 0; i-- { - current := fileLocations[i] - if i == len(fileLocations)-1 { - prev = current - continue - } - - if current.message == prev.message && current.location == prev.location && current.executedFunction == prev.executedFunction { - fileLocations[i].message = "" - } - prev = current - } finalErrorString := "" for _, fileLocation := range fileLocations { - if fileLocation.message == "" { - continue - } finalErrorString = finalErrorString + fileLocation.String() } + if strings.TrimSpace(finalErrorString) == "" { // Fallback to original error message if nothing was extracted return err From cdcf1bc6016593126b92c0eab09adaa55ed5e0e8 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sun, 16 Mar 2025 20:45:11 -0400 Subject: [PATCH 22/35] style: remove unnecessary break Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index d5c507eab..c1356e7c2 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -376,9 +376,6 @@ func cleanupExecError(filename string, err error) error { fileLocations := []TraceableError{} maxIterations := 100 for i := 0; i < maxIterations && current != nil; i++ { - if current == nil { - break - } if i == maxIterations-1 { return err } From cbdc22128eb5e01041311e30267beb0dbbea9c3b Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sun, 16 Mar 2025 20:45:31 -0400 Subject: [PATCH 23/35] refactor: use strings.Builder instead of string concatenation Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index c1356e7c2..46e75a1f3 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -422,17 +422,17 @@ func cleanupExecError(filename string, err error) error { current = errors.Unwrap(current) } - finalErrorString := "" + var finalErrorString strings.Builder for _, fileLocation := range fileLocations { - finalErrorString = finalErrorString + fileLocation.String() + fmt.Fprintf(&finalErrorString, "%s", fileLocation.String()) } - if strings.TrimSpace(finalErrorString) == "" { + if strings.TrimSpace(finalErrorString.String()) == "" { // Fallback to original error message if nothing was extracted return err } - return fmt.Errorf("%s", finalErrorString) + return fmt.Errorf("%s", finalErrorString.String()) } func sortTemplates(tpls map[string]renderable) []string { From 5202820f2f9a0c04514f2c15621491d398623ac4 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sun, 16 Mar 2025 20:53:18 -0400 Subject: [PATCH 24/35] refactor: define regexs at package scope Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 46e75a1f3..0dea01dd6 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -33,6 +33,14 @@ import ( chartutil "helm.sh/helm/v4/pkg/chart/v2/util" ) +// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=141 +// > "template: %s: executing %q at <%s>: %s" +var execErrFmt = regexp.MustCompile(`^template: (?P(?U).+): executing (?P(?U).+) at (?P(?U).+): (?P(?U).+)(?P( template:.*)?)$`) + +// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=138 +// > "template: %s: %s" +var execErrFmtWithoutTemplate = regexp.MustCompile(`^template: (?P(?U).+): (?P.*)(?P( template:.*)?)$`) + // Engine is an implementation of the Helm rendering implementation for templates. type Engine struct { // If strict is enabled, template rendering will fail if a template references @@ -344,20 +352,6 @@ func cleanupExecError(filename string, err error) error { return err } - // taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=138 - // > "template: %s: %s" - // taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=141 - // > "template: %s: executing %q at <%s>: %s" - - execErrFmt, compileErr := regexp.Compile(`^template: (?P(?U).+): executing (?P(?U).+) at (?P(?U).+): (?P(?U).+)(?P( template:.*)?)$`) - if compileErr != nil { - return err - } - execErrFmtWithoutTemplate, compileErr := regexp.Compile(`^template: (?P(?U).+): (?P.*)(?P( template:.*)?)$`) - if compileErr != nil { - return err - } - tokens := strings.SplitN(err.Error(), ": ", 3) if len(tokens) != 3 { // This might happen if a non-templating error occurs From 4f63c7335306f9197f6d560fbf3ccf3c5416e248 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sun, 16 Mar 2025 21:07:52 -0400 Subject: [PATCH 25/35] refactor: remove impractical safety check Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 0dea01dd6..3be69f1ab 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -421,11 +421,6 @@ func cleanupExecError(filename string, err error) error { fmt.Fprintf(&finalErrorString, "%s", fileLocation.String()) } - if strings.TrimSpace(finalErrorString.String()) == "" { - // Fallback to original error message if nothing was extracted - return err - } - return fmt.Errorf("%s", finalErrorString.String()) } From 4b9a9ecaf64acddea4f8a2cc5d22a8ad04d8b727 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sun, 16 Mar 2025 21:21:04 -0400 Subject: [PATCH 26/35] refactor: rename function and add doc-string Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 3be69f1ab..cc1951af7 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -312,7 +312,7 @@ func (e Engine) render(tpls map[string]renderable) (rendered map[string]string, 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{}, cleanupExecError(filename, err) + return map[string]string{}, reformatExecErrorMsg(filename, err) } // Work around the issue where Go will emit "" even if Options(missing=zero) @@ -347,7 +347,14 @@ type TraceableError struct { func (t TraceableError) String() string { return t.location + "\n " + t.executedFunction + "\n " + t.message + "\n" } -func cleanupExecError(filename string, err error) error { + +// reformatExecErrorMsg takes an error message for template rendering and formats it into a formatted +// multi-line error string +func reformatExecErrorMsg(filename string, err error) error { + // This function matches the error message against regex's for the text/template package. + // If the regex's can parse out details from that error message such as the line number, template it failed on, + // and error description, then it will construct a new error that displays these details in a structured way. + // If there are issues with parsing the error message, the err passed into the function should return instead. if _, isExecError := err.(template.ExecError); !isExecError { return err } From 782f6c640987f3df24b5e925e3bece4e7fdc17df Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 17 Mar 2025 12:37:18 -0400 Subject: [PATCH 27/35] refactor: shorten regex subexp syntax Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index cc1951af7..ad2c1679d 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -384,14 +384,10 @@ func reformatExecErrorMsg(filename string, err error) error { var traceable TraceableError if execErrFmt.MatchString(current.Error()) { matches := execErrFmt.FindStringSubmatch(current.Error()) - templateIndex := execErrFmt.SubexpIndex("templateName") - templateName := matches[templateIndex] - functionNameIndex := execErrFmt.SubexpIndex("functionName") - functionName := matches[functionNameIndex] - locationNameIndex := execErrFmt.SubexpIndex("location") - locationName := matches[locationNameIndex] - errMsgIndex := execErrFmt.SubexpIndex("errMsg") - errMsg := matches[errMsgIndex] + templateName := matches[execErrFmt.SubexpIndex("templateName")] + functionName := matches[execErrFmt.SubexpIndex("functionName")] + locationName := matches[execErrFmt.SubexpIndex("location")] + errMsg := matches[execErrFmt.SubexpIndex("errMsg")] traceable = TraceableError{ location: templateName, message: errMsg, @@ -399,10 +395,8 @@ func reformatExecErrorMsg(filename string, err error) error { } } else if execErrFmtWithoutTemplate.MatchString(current.Error()) { matches := execErrFmt.FindStringSubmatch(current.Error()) - templateIndex := execErrFmt.SubexpIndex("templateName") - templateName := matches[templateIndex] - errMsgIndex := execErrFmt.SubexpIndex("errMsg") - errMsg := matches[errMsgIndex] + templateName := matches[execErrFmt.SubexpIndex("templateName")] + errMsg := matches[execErrFmt.SubexpIndex("errMsg")] traceable = TraceableError{ location: templateName, message: errMsg, From 65084371c9a774c6c4c6afadd5f6fa2152b31b8a Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Mon, 17 Mar 2025 12:39:55 -0400 Subject: [PATCH 28/35] refactor: replace if MatchString with FindStringSubMatch Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index ad2c1679d..26f80a2f0 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -382,8 +382,7 @@ func reformatExecErrorMsg(filename string, err error) error { } var traceable TraceableError - if execErrFmt.MatchString(current.Error()) { - matches := execErrFmt.FindStringSubmatch(current.Error()) + if matches := execErrFmt.FindStringSubmatch(current.Error()); matches != nil { templateName := matches[execErrFmt.SubexpIndex("templateName")] functionName := matches[execErrFmt.SubexpIndex("functionName")] locationName := matches[execErrFmt.SubexpIndex("location")] @@ -393,8 +392,7 @@ func reformatExecErrorMsg(filename string, err error) error { message: errMsg, executedFunction: "executing " + functionName + " at " + locationName + ":", } - } else if execErrFmtWithoutTemplate.MatchString(current.Error()) { - matches := execErrFmt.FindStringSubmatch(current.Error()) + } else if matches := execErrFmtWithoutTemplate.FindStringSubmatch(current.Error()); matches != nil { templateName := matches[execErrFmt.SubexpIndex("templateName")] errMsg := matches[execErrFmt.SubexpIndex("errMsg")] traceable = TraceableError{ From ac98e977c392885c702515f4899b8f2a2c873870 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Tue, 18 Mar 2025 20:30:19 -0400 Subject: [PATCH 29/35] refactor: switch to while loop instead of for to reduce unnecessary variables Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 26f80a2f0..fee26490f 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -375,12 +375,7 @@ func reformatExecErrorMsg(filename string, err error) error { } current := err fileLocations := []TraceableError{} - maxIterations := 100 - for i := 0; i < maxIterations && current != nil; i++ { - if i == maxIterations-1 { - return err - } - + for current != nil { var traceable TraceableError if matches := execErrFmt.FindStringSubmatch(current.Error()); matches != nil { templateName := matches[execErrFmt.SubexpIndex("templateName")] From 48922e21d1e1647957ea7570d2b4c1fbf81283f5 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Tue, 18 Mar 2025 20:39:06 -0400 Subject: [PATCH 30/35] refactor: use struct equality instead of comparing each composition Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index fee26490f..7b0f87feb 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -399,9 +399,7 @@ func reformatExecErrorMsg(filename string, err error) error { } if len(fileLocations) > 0 { lastErr := fileLocations[len(fileLocations)-1] - if lastErr.message == traceable.message && - lastErr.location == traceable.location && - lastErr.executedFunction == traceable.executedFunction { + if lastErr == traceable { current = errors.Unwrap(current) continue } From 868cdc261ff645052c00b917b43e83187bb91a7f Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Tue, 18 Mar 2025 21:11:25 -0400 Subject: [PATCH 31/35] refactor: reduce flow-control operations Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 7b0f87feb..5b6999021 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -397,14 +397,9 @@ func reformatExecErrorMsg(filename string, err error) error { } else { return err } - if len(fileLocations) > 0 { - lastErr := fileLocations[len(fileLocations)-1] - if lastErr == traceable { - current = errors.Unwrap(current) - continue - } + if len(fileLocations) == 0 || fileLocations[len(fileLocations)-1] != traceable { + fileLocations = append(fileLocations, traceable) } - fileLocations = append(fileLocations, traceable) current = errors.Unwrap(current) } From 013f27c2947362a79f1c93a10add188e67c280fc Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sun, 20 Apr 2025 17:45:59 -0400 Subject: [PATCH 32/35] fix: use errors.New instead of fmt.Errorf Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 5b6999021..04feb43fe 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -408,7 +408,7 @@ func reformatExecErrorMsg(filename string, err error) error { fmt.Fprintf(&finalErrorString, "%s", fileLocation.String()) } - return fmt.Errorf("%s", finalErrorString.String()) + return errors.New(finalErrorString.String()) } func sortTemplates(tpls map[string]renderable) []string { From 0e0a8cc76534177a1c9809804232ac334d7f0f0e Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Wed, 23 Apr 2025 21:56:30 -0400 Subject: [PATCH 33/35] fix: address no-template-associated type of error Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 20 +++++++++++++++++++- pkg/engine/engine_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 04feb43fe..7c92b4505 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -41,6 +41,10 @@ var execErrFmt = regexp.MustCompile(`^template: (?P(?U).+): execut // > "template: %s: %s" var execErrFmtWithoutTemplate = regexp.MustCompile(`^template: (?P(?U).+): (?P.*)(?P( template:.*)?)$`) +// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=191 +// > "template: no template %q associated with template %q" +var execErrNoTemplateAssociated = regexp.MustCompile(`^template: no template (?P.*) associated with template (?P(.*)?)$`) + // Engine is an implementation of the Helm rendering implementation for templates. type Engine struct { // If strict is enabled, template rendering will fail if a template references @@ -345,7 +349,17 @@ type TraceableError struct { } func (t TraceableError) String() string { - return t.location + "\n " + t.executedFunction + "\n " + t.message + "\n" + var errorString strings.Builder + if t.location != "" { + fmt.Fprintf(&errorString, "%s\n ", t.location) + } + if t.executedFunction != "" { + fmt.Fprintf(&errorString, "%s\n ", t.executedFunction) + } + if t.message != "" { + fmt.Fprintf(&errorString, "%s\n", t.message) + } + return errorString.String() } // reformatExecErrorMsg takes an error message for template rendering and formats it into a formatted @@ -394,6 +408,10 @@ func reformatExecErrorMsg(filename string, err error) error { location: templateName, message: errMsg, } + } else if matches := execErrNoTemplateAssociated.FindStringSubmatch(current.Error()); matches != nil { + traceable = TraceableError{ + message: current.Error(), + } } else { return err } diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 58d8588ab..5359465d4 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -22,6 +22,7 @@ import ( "strings" "sync" "testing" + "text/template" "github.com/stretchr/testify/assert" @@ -1336,6 +1337,40 @@ NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:50 assert.Equal(t, expectedErrorMessage, err.Error()) } +func TestMultilineNoTemplateAssociatedError(t *testing.T) { + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "multiline"}, + Templates: []*chart.File{ + {Name: "templates/svc.yaml", Data: []byte( + `name: {{ include "nested_helper.name" . }}`, + )}, + {Name: "templates/test.yaml", Data: []byte( + `{{ toYaml .Values }}`, + )}, + {Name: "charts/common/templates/_helpers_2.tpl", Data: []byte( + `{{ toYaml .Values }}`, + )}, + }, + } + + expectedErrorMessage := `multiline/templates/svc.yaml:1:9 + executing "multiline/templates/svc.yaml" at : + error calling include: +template: no template "nested_helper.name" associated with template "gotpl" +` + + v := chartutil.Values{} + + val, _ := chartutil.CoalesceValues(c, v) + vals := map[string]interface{}{ + "Values": val.AsMap(), + } + _, err := Render(c, vals) + + assert.NotNil(t, err) + assert.Equal(t, expectedErrorMessage, err.Error()) +} + func TestRenderCustomTemplateFuncs(t *testing.T) { // Create a chart with two templates that use custom functions c := &chart.Chart{ From d10c5f642901946d9bf89e01e29a85ffdd97c690 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Wed, 23 Apr 2025 22:07:21 -0400 Subject: [PATCH 34/35] style: trim space from formatted error messages Signed-off-by: Jesse Simpson --- pkg/engine/engine.go | 2 +- pkg/engine/engine_test.go | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 7c92b4505..009b5e432 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -426,7 +426,7 @@ func reformatExecErrorMsg(filename string, err error) error { fmt.Fprintf(&finalErrorString, "%s", fileLocation.String()) } - return errors.New(finalErrorString.String()) + return errors.New(strings.TrimSpace(finalErrorString.String())) } func sortTemplates(tpls map[string]renderable) []string { diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 5359465d4..e99fac2e8 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -1322,8 +1322,7 @@ NestedHelperFunctions/templates/_helpers_1.tpl:1:39 error calling include: NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:50 executing "common.names.get_name" at <.Release.Name>: - nil pointer evaluating interface {}.Name -` + nil pointer evaluating interface {}.Name` v := chartutil.Values{} @@ -1356,8 +1355,7 @@ func TestMultilineNoTemplateAssociatedError(t *testing.T) { expectedErrorMessage := `multiline/templates/svc.yaml:1:9 executing "multiline/templates/svc.yaml" at : error calling include: -template: no template "nested_helper.name" associated with template "gotpl" -` +template: no template "nested_helper.name" associated with template "gotpl"` v := chartutil.Values{} From e0a67b1028dc175aafa2116eb3a17ce786cbc8cd Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Thu, 24 Apr 2025 13:08:30 -0400 Subject: [PATCH 35/35] test: use more realistic unit-test scenario by not relying on Release.Name Signed-off-by: Jesse Simpson --- pkg/engine/engine_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index e99fac2e8..f4228fbd7 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -1309,7 +1309,7 @@ func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { `{{- define "nested_helper.name" -}}{{- include "common.names.get_name" . -}}{{- end -}}`, )}, {Name: "charts/common/templates/_helpers_2.tpl", Data: []byte( - `{{- define "common.names.get_name" -}}{{- .Release.Name | trunc 63 | trimSuffix "-" -}}{{- end -}}`, + `{{- define "common.names.get_name" -}}{{- .Values.nonexistant.key | trunc 63 | trimSuffix "-" -}}{{- end -}}`, )}, }, } @@ -1320,9 +1320,9 @@ func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { NestedHelperFunctions/templates/_helpers_1.tpl:1:39 executing "nested_helper.name" at : error calling include: -NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:50 - executing "common.names.get_name" at <.Release.Name>: - nil pointer evaluating interface {}.Name` +NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:49 + executing "common.names.get_name" at <.Values.nonexistant.key>: + nil pointer evaluating interface {}.key` v := chartutil.Values{}