fix: insert newline after doc separators glued to content by template trimming#31868
Conversation
pull-request-size
Bot
added
the
size/L
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where Go template whitespace trimming ({{-) could produce invalid YAML by concatenating the document separator --- directly with content (e.g., ---apiVersion), which violates the YAML specification requiring separators to be on their own line.
Changes:
- Introduced
stripLeadingDocSeparatorsfunction to remove leading---separators from rendered templates - Integrated stripping logic into
annotateAndMergebefore parsing YAML documents - Added comprehensive test coverage for various separator scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/action/action.go | Implements stripLeadingDocSeparators function and integrates it into the annotateAndMerge workflow to handle malformed document separators |
| pkg/action/action_test.go | Adds unit tests for stripLeadingDocSeparators and integration tests for annotateAndMerge covering the regression case |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2b40cd8 to
164409a
Compare
matheuscscp
changed the title
---\n{{- something }}\napiVersion degenerating to ---apiVersion
|
@banjoh @gjenkins8 Can you please review? 4.1.2 is coming soon |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
… trimming Signed-off-by: Matheus Pimenta <[email protected]>
There was a problem hiding this comment.
@matheuscscp thanks for the fix. Note that this is a Helm 4 only regression from Helm 3, so we should ship this in the next Helm 4 patch release.
Also addressed change requested by @TerryHowe and @banjoh in #31868 (comment).
As a follow-up, I would however like to propose that in Charts apiVersion v3 we drop support for special handling around gotemplate whitespace trimming for yaml separators. In general, when a {{- is used, all leading white space should be trimmed from the immediately following text - including "space, horizontal tab, carriage return, and newline" (see https://pkg.go.dev/text/template#hdr-Text_and_spaces). I do not yet know why we handle this special case as opposed to asking chart authors to fix this. We do have to support this for existing apiVersion v2 charts for backwards compatibility (both with Helm 3 and Helm 4), but Charts apiVersion v3 could simplify our code by not handling this in a special way for YAML separators. We would need to add this to a charts v2 -> v3 migration guide if we do.
| remaining = remaining[idx+3:] | ||
| // If "---" is followed by non-whitespace (e.g. "---apiVersion"), | ||
| // insert a newline to make it a proper document separator. | ||
| if len(remaining) > 0 && remaining[0] != '\n' && remaining[0] != '\r' && remaining[0] != ' ' && remaining[0] != '\t' { |
There was a problem hiding this comment.
nice. note that this conforms to go's definition of whitespace trimming in templating context: https://pkg.go.dev/text/template#hdr-Text_and_spaces
For this trimming, the definition of white space characters is the same as in Go: space, horizontal tab, carriage return, and newline.
scottrigby
added
needs-pick
@scottrigby We discussed in the Flux dev meeting today about this breaking change in Chart API v3. It was noted that this kind of behavior should not happen only when using postrenderers. The behavior fixed by this PR should be caught as an error for example on |
@matheuscscp yes, without question 👍 agree that any discrepancy between the two would be a bug, regardless of the expected behavior. |
Fixes: #31867