fix: do not treat consecutive groups of 3 dashes in a line as document separators#31936
fix: do not treat consecutive groups of 3 dashes in a line as document separators#31936marvin-roesch wants to merge 2 commits into
Conversation
…t separators Signed-off-by: Marvin Rösch <[email protected]>
pull-request-size
Bot
added
the
size/S
There was a problem hiding this comment.
Pull request overview
This PR refines Helm’s rendered-manifest post-processing to avoid misinterpreting consecutive --- groups that appear within a line (e.g., in script output or values) as YAML document separators, addressing the reported regression in #31935.
Changes:
- Update
fixDocSeparatorsto consume all consecutive-characters after a non-line-start---, preventing the next scan from treating leftover dashes as a line-start separator. - Add a regression test ensuring multiple
---groups in a value (e.g.,---------value) are not treated as document separators.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pkg/action/action.go |
Adjusts the doc-separator fix-up logic to skip consecutive dashes when --- is not at the start of a line. |
pkg/action/action_test.go |
Adds a targeted regression test for consecutive triple-dash groups within a value. |
💡 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.
Co-authored-by: Terry Howe <[email protected]> Signed-off-by: Marvin Rösch <[email protected]>
There was a problem hiding this comment.
Pull request overview
Fixes a regression in fixDocSeparators where consecutive --- groups occurring mid-line (e.g., in values or script output) could be misinterpreted as YAML document separators after earlier processing.
Changes:
- Update
fixDocSeparatorsto consume all consecutive-characters when a found---is not at the start of a line, preventing subsequent iterations from treating remaining dashes as a separator. - Add test cases covering multiple consecutive triple-dash groups in values and a block scalar containing long dash sequences.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/action/action.go | Adjusts separator-fix logic to avoid misclassifying contiguous dash runs mid-line as document separators. |
| pkg/action/action_test.go | Adds regression tests ensuring mid-line dash runs and block-scalar content remain unchanged. |
💡 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.
| name: "block scalar with dashes in quoted string", | ||
| input: "---\napiVersion: v1\nkind: ConfigMap\ndata:\n script.sh: |\n echo \"------------------------------------\"\n", | ||
| expected: "---\napiVersion: v1\nkind: ConfigMap\ndata:\n script.sh: |\n echo \"------------------------------------\"\n", |
|
Please hold merging this, we've found the regex from Helm v1 that does the right thing when splitting |
Here: #31941 |
|
Closing this in favour of the proper fix. |
What this PR does / why we need it:
Fixes #31935. We consume all remaining dashes after encountering
---that is not at the start of a line such that the next iteration does not treat a---preceded by another---as another document separator because it appears to be at the start of the remaining content.Note that there conceivably could be contrived scenarios where this logic fails. However, this errs on the side of realistic scenarios, as recovering the original intent from whitespace-trimmed output is generally ambiguous.
The following template demonstrates this ambiguity fully:
After whitespace trimming, it would like this:
------key: value---another-doc-key: valueBoth by the current fix logic implemented in #31868 and this new one, this would get reconstructed as follows:
I am not sure there is a good way to resolve such ambiguities in an elegant way.