fix: Improve error reporting for helm template --debug with --show-only - #31185
Conversation
| } | ||
| if missing { | ||
| return fmt.Errorf("could not find template %s in chart", f) | ||
| if err != nil && settings.Debug { |
There was a problem hiding this comment.
This would probably clearer and more reliable if you saved the error above like saveError := err and then no need to check settings.Debug. I get this works now, but someone might come along and not realize that err gets used way down here later.
489a06e to
da7a429
Compare
|
Hey, waiting for this 🫡 |
TerryHowe
left a comment
There was a problem hiding this comment.
Please fix the lint error.
There was a problem hiding this comment.
Pull request overview
Fixes misleading error reporting for helm template --debug --show-only by preserving and prioritizing the original render/install error over the downstream “could not find template” message when rendering fails (closes #31183).
Changes:
- Preserve the original
runInstall()error intemplatecommand flow and return it when--show-onlycannot find rendered output due to a render failure. - Add a new regression test case for
--debug --show-onlywhen the targeted template has an invalid template expression. - Introduce a dedicated test chart and golden output capturing the corrected error behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/template.go | Preserve runInstall() error and prefer it over misleading --show-only missing-template error in debug render-failure scenarios. |
| pkg/cmd/template_test.go | Add regression test for template --debug --show-only when template rendering fails. |
| pkg/cmd/testdata/testcharts/chart-with-template-with-invalid-template-expr/Chart.yaml | New test chart metadata for invalid template expression scenario. |
| pkg/cmd/testdata/testcharts/chart-with-template-with-invalid-template-expr/README.md | New test chart readme (mirrors existing testchart structure). |
| pkg/cmd/testdata/testcharts/chart-with-template-with-invalid-template-expr/templates/alpine-pod.yaml | New template designed to trigger a render-time template execution error. |
| pkg/cmd/testdata/testcharts/chart-with-template-with-invalid-template-expr/values.yaml | Values for the new test chart. |
| pkg/cmd/testdata/output/template-with-invalid-template-expr-debug-show-only.txt | New golden file asserting corrected error output for the regression case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if savedErr != nil && settings.Debug { | ||
| // assume the manifest itself is too malformed to be rendered | ||
| return savedErr |
There was a problem hiding this comment.
The inline comment about the savedErr branch is misleading: the code isn’t detecting a “malformed manifest”, it’s prioritizing the original render/install error over a potentially misleading --show-only “could not find template” error (because the file list is derived from rendered output). Consider rewording the comment to reflect that rationale so future readers don’t think there’s a manifest-structure check here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@TerryHowe Could you please approve the CI run so we can proceed with merging? |
| } | ||
| return err | ||
| } | ||
| savedErr := err |
There was a problem hiding this comment.
| savedErr := err | |
| installErr := err |
…ow-only` Signed-off-by: Jeaeun Kim <me@kyoku.dev>
Signed-off-by: Jeaeun Kim <me@kyoku.dev>
Signed-off-by: Jeaeun Kim <me@kyoku.dev>
Signed-off-by: Jeaeun Kim <me@kyoku.dev>
| @@ -0,0 +1,8 @@ | |||
| apiVersion: v1 | |||
| sources: | ||
| - https://github.com/helm/helm | ||
| version: 0.1.0 | ||
| type: application |
| @@ -0,0 +1,13 @@ | |||
| #Alpine: A simple Helm chart | |||
| The `values.yaml` file contains the default values for the | ||
| `alpine-pod.yaml` template. | ||
|
|
||
| You can install this example using `helm install ./alpine`. |
| if missing { | ||
| if installErr != nil && settings.Debug { | ||
| // assume the manifest itself is too malformed to be rendered | ||
| return installErr | ||
| } |
| @@ -0,0 +1,13 @@ | |||
| #Alpine: A simple Helm chart | |||
|
|
||
| Run a single pod of Alpine Linux. | ||
|
|
||
| This example was generated using the command `helm create alpine`. |
| The `values.yaml` file contains the default values for the | ||
| `alpine-pod.yaml` template. | ||
|
|
||
| You can install this example using `helm install ./alpine`. |
| } | ||
| return err | ||
| } | ||
| installErr := err |
helm template --debug with --show-onlyhelm template --debug with --show-only
closes #31183
What this PR does / why we need it:
This PR fixes a bug where
helm template --debug --show-onlywould show a misleading "could not find template" error when the target template had a rendering error.Currently, the
--debugflag suppresses the initial template rendering error and then later reports the misleading error, which hides the true cause of the failure. This change ensures that the original rendering error is captured and prioritized, providing users with accurate and actionable feedback.Special notes for your reviewer:
The core of the fix is in the error handling logic after the templates are rendered. I've introduced a change to preserve the error from the
runInstall()function, even in debug mode. The final error report now checks for this rendering error first before checking if the file specified in--show-onlyexists in the manifest output.If applicable:
docs neededlabel should be applied if so)