chore(templating): drop glued-separator workaround for chart v3#32185
Open
texasich wants to merge 2 commits into
Open
chore(templating): drop glued-separator workaround for chart v3#32185texasich wants to merge 2 commits into
texasich wants to merge 2 commits into
Conversation
For chart apiVersion v3, tighten SplitManifests so it no longer silently
splits `---` glued to content (as happens when `{{-` trims the newline
after `---`). The Chart v1/v2 path in pkg/release/v1/util keeps the
lenient behaviour for backwards compatibility.
The regex now requires `---` to be alone on a line (optional trailing
horizontal whitespace, then newline or end of stream) before it counts as
a document separator. Glued input like `---apiVersion: v1` stays on the
document body so downstream YAML parsing surfaces the problem, matching
standard Go template semantics for `{{-`.
Refs helm#32036
Signed-off-by: texasich <texasich@users.noreply.github.com>
Signed-off-by: texasich <texasich@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR tightens Chart API v3 manifest splitting semantics by treating YAML separators glued to content as part of the document, and expands tests to cover additional separator edge-cases (trailing separators, whitespace, CRLF).
Changes:
- Make
SplitManifestsuse a stricter YAML document separator regex aligned with Chart API v3 behavior. - Update/rename existing tests to assert glued separators are not split.
- Add new tests for trailing separators, trailing horizontal whitespace, and CRLF line endings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/release/v2/util/manifest.go | Stricter separator regex + updated Chart v3 behavior documentation. |
| internal/release/v2/util/manifest_test.go | Updates expectations for glued separators and adds new edge-case tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+37
to
+47
| // sep matches YAML document separators. A separator is `---` at the start of | ||
| // a line (or start of the stream), optionally followed by trailing horizontal | ||
| // whitespace, and then a newline or end of stream. | ||
| // | ||
| // This is intentionally stricter than the Chart v1/v2 regex in | ||
| // pkg/release/v1/util/manifest.go. The v1/v2 version tolerates | ||
| // `---<non-whitespace>` (e.g. `---apiVersion: v1`) by treating it as a | ||
| // separator glued to content — a silent correction for Go template whitespace | ||
| // trimming (`{{-`) eating the newline after `---`. Chart v3 does not carry | ||
| // that workaround forward; see the function comment below. | ||
| var sep = regexp.MustCompile("(?:^|\\s*\n)---[ \\t]*(?:\\r?\\n|$)") |
|
📝 Documentation Update I've updated the docs to cover this Chart v3 template separator behavior change. Docs PR: helm/helm-www#2083 The migration guide ( 🤖 Generated with Promptless |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #32036.
Rebased version of #32062 (original closed by author after inactivity; this is the refreshed implementation on current
main).Saw this one sitting open and figured it was a good small cleanup. The scope in the issue is pretty contained so I kept the diff to one package.
What
Tightens
SplitManifestsininternal/release/v2/util(the chart v3 path) so it no longer silently splits a---that's glued to content. The regex now requires---to be alone on a line — optional trailing spaces/tabs, then a newline or end of stream — before it counts as a document separator.Input like:
(which is what
{{- include ... }}after---produces) used to be split back into two documents by the regex---\s*. With this change it stays on the document body and the downstream YAML parser can surface the problem — which is exactly the point of the issue.What's NOT changed
The Chart v1/v2 path in
pkg/release/v1/util/manifest.gois left alone on purpose. Per the issue, v1/v2 chart compatibility has to be preserved indefinitely — a huge number of charts rely on the silent correction and breaking them would be a nasty regression. The strict behaviour is a v3-only, opt-in thing.Tests
Updated
TestSplitManifestsin the v2 util package: the four existing "glued separator" cases now assert that the glued---<content>stays as part of the same document. Added a few extra cases to pin down the edges:---at end of input with no trailing newline still counts as a separator---with trailing spaces/tabs still countsgo test ./internal/release/... ./pkg/release/...is green. Build and vet pass.A follow-up migration note for the v2 → v3 guide on helm-www would be worth adding; happy to do that separately if a maintainer wants.
AI disclosure
This contribution was prepared with assistance from an AI coding agent (Hermes Agent). The human reviewed the diff, verified tests locally, performed the rebase, and is responsible for the changes.