Move to yaml.v3 and preserve comments, multiline strings, anchors in helm values #1055
Conversation
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
bewinsnw
requested review from
jannfis,
chengfang,
pasha-codefresh and
ishitasequeira
as code owners
bewinsnw
force-pushed
the
bazzargh/yamlv3
branch
from
9813194 to
5531d2d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1055 +/- ##
==========================================
+ Coverage 62.89% 63.37% +0.47%
==========================================
Files 15 15
Lines 2269 2326 +57
==========================================
+ Hits 1427 1474 +47
- Misses 753 760 +7
- Partials 89 92 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
In order to support round-tripping of files with multiline yaml and anchors, move to yaml.v3. This lower-level interface preserves comments and doesn't follow aliases by default. Support for aliases in Helm values files is fairly minimal and I need to add tests to demonstrate that this works; it only follows one level of alias to avoid getting stuck in loops. A lot of tests needed updated because it seems yaml.v3 switched to four spaces instead of two when marshalling by default. Signed-off-by: Brian Ewins <bewins@nerdwallet.com>
This change makes for less verbose code, also what we care about is the transformation of the yaml, not the internal representation. Signed-off-by: Brian Ewins <bewins@nerdwallet.com>
When round-tripping helm yaml, we want comments to be preserved. Multiline strings should retain their formatting (this is to allow for, for example, scripts in k8s commands). Aliases should be preserved, and followed, both for mappings and scalars. Signed-off-by: Brian Ewins <bewins@nerdwallet.com>
When yaml.v3 parses yaml, string scalars are tagged "!!str". It is possible that we could parse a values file which sets the version to eg 1.23 (a float), so that when we later update the value to "v2.4" it can't be serialized. To protect against this, set the tag whenever we create or update scalars that are explicitly intended to be strings. Signed-off-by: Brian Ewins <bewins@nerdwallet.com>
bewinsnw
force-pushed
the
bazzargh/yamlv3
branch
from
2318e99 to
91b359f
Compare
|
Given this PR seems simpler than #1039, doesn't introduce a new dependency (just a version bump) and that there was no feedback on the review comments for about two weeks, I'd be fine to go with this one instead of #1039 for now. @chengfang WDYT? |
|
Looks promising. I'll take a closer look. |
chengfang
reviewed
Mar 5, 2025
yaml.v3 changed the default indent to 4 spaces, see: go-yaml/yaml@3c39489 in order to avoid unnecessary whitespace changes in code previously formatted with argo_image_updater, use a 2-space indent instead. Signed-off-by: Brian Ewins <bewins@nerdwallet.com>
bewinsnw
force-pushed
the
bazzargh/yamlv3
branch
from
9d5d843 to
b49a6c3
Compare
It's possible to preserve the indent in the original yaml when
we Unmarshal to a yaml.Node, and the structure of the original
yaml allows us to guess the indent.
The two easiest cases are when we have a nested block-style mapping,
or a sequence inside a mapping and the sequence is indented. For
those, the node tells us the parsed column (starting at 1) and we
use that indent.
However: if we parse to some other data structure (as we do with
Kustomize) or the original yaml has some structure where the indentation
is unclear, eg:
root:
- unguessable indent
- deeper:
guessable: but requires deeper traversal
... then we just guess '2'.
This should generate _fewer_ useless whitespace changes, but cannot
prevent them all.
Signed-off-by: Brian Ewins <bewins@nerdwallet.com>
bewinsnw
force-pushed
the
bazzargh/yamlv3
branch
from
adc8d7e to
b43859d
Compare
chengfang
approved these changes
Mar 6, 2025
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.
Similar to #1039 (and conflicts with it). In our codefresh install developers have complained that various parts of the original yaml aren't preserved:
In particular we have cases where multiple jobs are defined in a chart which use the same images, which should all be updated via a single reference. yaml.v2 doesn't support this, but yaml.v3 does; the Node structure preserves pre-, post- and end-of-line comments, keeps the string formatting marker with the string for marshalling, and allows us to navigate aliases without losing them.
In this PR, I first switched to yaml.v3, then, like #1039, simplified the test cases for this code to not use the internal structures we unmarshal to but instead do yaml-to-yaml comparisons; this is easier to read and maintain and less code. Finally I added tests for the features described above.
I've only supported one level of aliasing; you can have multiple aliases through the path as you navigate the document but you can't have a mapping which is an alias for a mapping which is an alias for a mapping. Supporting that may mean the code could get stuck in a loop, and it doesn't seem like a sensible document structure anyway, just make the direct reference?
I don't currently support or set tags in the yaml, eg !!str. This would be an issue if you try to set a field which is not tagged as a string.
Unlike #1039, this does not also update the path syntax used for updates.