Skip to content

Preserve formatting in Helm values files #1166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

grahamalama
Copy link
Contributor

@grahamalama grahamalama commented Jun 14, 2025

Welp, I spent quite some time writing this PR to preserve formatting in Helm values files only to discover that there was already some activity in this area, namely #1039 and this branch referenced in that PR.

You can consider this another implementation and maybe decide between the two approaches. There's still more polish I could add, but since I discovered #1039, I thought I'd get this PR up to see if I should keep working on it.

Whether this PR or #1039, it'd be nice to get this general feature of preserving the formatting of Helm values files in.

@grahamalama grahamalama force-pushed the edit-helm-yaml-formatting branch from 9438961 to d67d921 Compare June 14, 2025 03:03
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2025

Codecov Report

Attention: Patch coverage is 83.83234% with 27 lines in your changes missing coverage. Please review.

Project coverage is 62.74%. Comparing base (889af4b) to head (219b67a).

Files with missing lines Patch % Lines
pkg/argocd/update.go 83.83% 19 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1166      +/-   ##
==========================================
- Coverage   63.27%   62.74%   -0.53%     
==========================================
  Files          15       15              
  Lines        2358     2365       +7     
==========================================
- Hits         1492     1484       -8     
- Misses        771      782      +11     
- Partials       95       99       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Graham Beckley <[email protected]>
Signed-off-by: Graham Beckley <[email protected]>
@chengfang
Copy link
Collaborator

Thanks for the contribution, @grahamalama. PRs #1055 and #1062, based on #1039, were merged and included in v0.16.0.

@grahamalama
Copy link
Contributor Author

grahamalama commented Jun 30, 2025

@chengfang ahh I see. So it seems the final version of the formatting changes you mentioned do not use goccy/go-yaml? One consequence of this is that blank lines are not preserved. I've also noticed that image updater (whether through the yaml library or our logic) enforces this opinionated formatting of array items:

array:
- item

vs something like:

array:
  - item

goccy/go-yaml seems to handle both of these things, so I see some benefit of adopting that library as it would lead to overall less noisy diffs when applying updates.

What should I do with this PR? I'm sure it's not desirable to have 2 different yaml libraries in use simultaneously in the project, but if using goccy/go-yaml is something you want to consider further, I can continue working on this PR so that we switch everything over to goccy/go-yaml (or maybe create a separate follow-on PR to handle that). What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants