fix: make toToml swallow errors consistent with toYaml and toJson#31915
fix: make toToml swallow errors consistent with toYaml and toJson#31915realgagangupta wants to merge 2 commits intohelm:mainfrom
Conversation
pull-request-size
bot
added
the
size/XS
There was a problem hiding this comment.
Pull request overview
Aligns Helm template helper behavior by making toToml swallow TOML marshal/encode errors (returning "") to match toYaml/toJson, avoiding raw error strings being rendered into templates.
Changes:
- Update
toTOMLto return an empty string on encode errors instead oferr.Error(). - Clarify
toTOMLbehavior in its doc comment (errors are swallowed, consistent with YAML/JSON helpers).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // toTOML takes an interface, marshals it to toml, and returns a string. It will | ||
| // always return a string, even on marshal error (empty string). | ||
| // Errors are swallowed, consistent with toYAML and toJSON behavior. | ||
| // | ||
| // This is designed to be called from a template. | ||
| func toTOML(v any) string { |
There was a problem hiding this comment.
The PR description says users can use mustToToml for error visibility, but this repo currently exposes only toToml (mapped to toTOML) and there is no mustToToml equivalent in funcMap() or elsewhere. Either add a mustToTOML/mustToToml function and wire it into the func map (mirroring mustToYaml/mustToJson), or adjust the PR description/docs to avoid referencing a non-existent function.
| func toTOML(v any) string { | ||
| b := bytes.NewBuffer(nil) | ||
| e := toml.NewEncoder(b) | ||
| err := e.Encode(v) | ||
| if err != nil { | ||
| return err.Error() | ||
| // Swallow errors inside of a template. | ||
| return "" | ||
| } |
There was a problem hiding this comment.
This changes toTOML to swallow encoding errors, but there is no corresponding test asserting the new behavior (unlike toYaml/toJson, which have explicit “swallow error” cases in funcs_test.go). Add a test case that forces TOML encoding to fail (e.g., a cyclic structure) and asserts toToml renders an empty string.
pull-request-size
bot
added
size/S
Previously toToml returned err.Error() on failure, rendering the error message string into the template output. toYAML and toJSON both return an empty string on error to swallow it silently inside templates. This aligns toToml behavior with the other serialization functions. Fixes: helm#31430 Signed-off-by: Gagan Gupta <realgagangupta@users.noreply.github.com>
Add a test case that verifies toToml returns "" on encoding errors (e.g. map[int]string where keys are not strings), consistent with how toYaml and toJson handle errors. Signed-off-by: Gagan Gupta <realgagangupta@users.noreply.github.com>
realgagangupta
force-pushed
the
fix-toToml-error-return
branch
from
e3d9db0 to
68097e3
Compare
|
Description is still talking about the non existent mustToToml |
|
Good catch! Updated the description — |
Problem
`toToml` returns `err.Error()` on failure, which renders the raw error message string directly into the template output. This is inconsistent with `toYAML` and `toJSON`, both of which swallow errors silently by returning `""`.
Before:
```go
if err != nil {
return err.Error() // renders error text into template
}
```
After:
```go
if err != nil {
// Swallow errors inside of a template.
return "" // consistent with toYAML and toJSON
}
```
Solution
Changed `toTOML` to return `""` on error, matching the behavior of `toYAML` and `toJSON`.
Fixes #31430