Skip to content

fix(helm): preserve block scalar semantics#4541

Merged
brandtkeller merged 3 commits intomainfrom
4537_configmap_newline
Jan 23, 2026
Merged

fix(helm): preserve block scalar semantics#4541
brandtkeller merged 3 commits intomainfrom
4537_configmap_newline

Conversation

@brandtkeller
Copy link
Copy Markdown
Member

@brandtkeller brandtkeller commented Jan 22, 2026 •
edited
Loading

Description

v0.63.0 introduced new behaviors to the post-render process for helm charts whereby:

  • Helm's releaseutil.SortManifests strips trailing newlines from resource.Content (this has always been the case)
  • When YAML with a block scalar (data: |) is parsed without a trailing document newline, the parser doesn't preserve the trailing newline in the scalar's value
  • When re-marshaled, the library outputs data: |- (with chomp indicator), which explicitly removes the trailing newline

This change ensures we preserve the trailing newline.

Related Issue

Fixes #4537

Checklist before merging

@brandtkeller
fix(helm): preserve block scalar semantics
b24f30d 
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
@brandtkeller brandtkeller self-assigned this Jan 22, 2026
@brandtkeller brandtkeller requested review from a team as code owners January 22, 2026 00:12
@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 22, 2026 •
edited
Loading

✅ Deploy Preview for zarf-docs ready!

Name Link
🔨 Latest commit 6fd7703
🔍 Latest deploy log https://app.netlify.com/projects/zarf-docs/deploys/69728f60686ac90008441429
😎 Deploy Preview https://deploy-preview-4541--zarf-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 22, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 35.29412% with 22 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/internal/packager/helm/post-render.go 35.29% 19 Missing and 3 partials ⚠️
Files with missing lines Coverage Δ
src/internal/packager/helm/post-render.go 4.72% <35.29%> (+4.72%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AustinAbro321
Copy link
Copy Markdown
Member

AustinAbro321 commented Jan 22, 2026

@brandtkeller Did you find the PR or dependency update that changed this behavior?

@brandtkeller
Copy link
Copy Markdown
Member Author

brandtkeller commented Jan 22, 2026

Did you find the PR or dependency update that changed this behavior?

#4194 introduced an additional marshal into the editHelmResources which didn't happen previously and leads to the sequence in the description.

@AustinAbro321
Copy link
Copy Markdown
Member

AustinAbro321 commented Jan 22, 2026

Could you add a test for this? E2E or unit if you can make that work

@AustinAbro321
Copy link
Copy Markdown
Member

AustinAbro321 commented Jan 22, 2026 •
edited
Loading

What do you think of using kyaml "sigs.k8s.io/kustomize/kyaml/yaml" here instead. I tested it out and it seems to solve this problem in a cleaner way. Though I have not run the full e2e test suite with it https://kubernetes.io/docs/reference/encodings/kyaml/

@brandtkeller
Copy link
Copy Markdown
Member Author

brandtkeller commented Jan 22, 2026

What do you think of using kyaml "sigs.k8s.io/kustomize/kyaml/yaml" here instead. I tested it out and it seems to solve this problem in a cleaner way. Though I have not run the full e2e test suite with it https://kubernetes.io/docs/reference/encodings/kyaml/

I'll get a test written to serve as a baseline and then look at swapping that dependency - I should have put this in draft as it was end-of-day and not complete (mainly wanted CI).

@brandtkeller brandtkeller marked this pull request as draft January 22, 2026 14:50
@AustinAbro321
Copy link
Copy Markdown
Member

AustinAbro321 commented Jan 22, 2026

Great! I rescind my recommendation to switch to "sigs.k8s.io/kustomize/kyaml/yaml", I was conflating kustomize yaml with kyaml. The latter seems to be more of a tool for printing clearly rather than marshalling and unmarshalling

brandtkeller and others added 2 commits January 22, 2026 11:16
@brandtkeller brandtkeller marked this pull request as ready for review January 22, 2026 20:58
@brandtkeller
Copy link
Copy Markdown
Member Author

brandtkeller commented Jan 22, 2026

Great! I rescind my recommendation to switch to "sigs.k8s.io/kustomize/kyaml/yaml", I was conflating kustomize yaml with kyaml. The latter seems to be more of a tool for printing clearly rather than marshalling and unmarshalling

Composed the logic into a new private function to make it easier to test (initial and regression). You can see the test fail or pass based on the existence of re-adding the newline.

I don't see any changes in behavior across yaml parsers but do largely believe that this is a combination of the newline being stripped prior to unmarshall/marshall. I see kyaml as an example of having similarly implemented a check for re-adding the newline.

AustinAbro321
AustinAbro321 approved these changes Jan 23, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests, help me understand what block scalers are

@brandtkeller brandtkeller added this pull request to the merge queue Jan 23, 2026
Merged via the queue into main with commit 8655c1c Jan 23, 2026
30 checks passed
@brandtkeller brandtkeller deleted the 4537_configmap_newline branch January 23, 2026 17:57
@github-project-automation github-project-automation Bot moved this to Done in Zarf Jan 23, 2026
chaospuppy pushed a commit to chaospuppy/zarf that referenced this pull request Jan 23, 2026
@brandtkeller @chaospuppy
fix(helm): preserve block scalar semantics (zarf-dev#4541)
8649e36 
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
@zarf-release-please zarf-release-please Bot mentioned this pull request Jan 28, 2026
Merged
AustinAbro321 pushed a commit that referenced this pull request Feb 4, 2026
@brandtkeller @AustinAbro321
fix(helm): preserve block scalar semantics (#4541)
fb4572e 
Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AustinAbro321 AustinAbro321 AustinAbro321 approved these changes

Assignees

@brandtkeller brandtkeller

Labels

None yet

Projects

Zarf
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Configmaps Missing Trailing Newlines

2 participants

@brandtkeller @AustinAbro321