Skip to content
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

feat: make yaml-update syntax the same as json-update syntax #3156

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

krancour
Copy link
Member

@krancour krancour commented Dec 19, 2024

This complements #3151.

#3151 uses tidwall/sjson for modifying JSON documents whilst preserving their existing formatting.

The keys/paths supported by that library use simple dot-separated notation. Dots can be escaped. Colons are used as a hint that a numeric-looking key part should be treated as a key in an object and not as an index in a sequence. Colons also can be escaped.

Examples:

  • foo.bar --> [foo bar]
  • foo.bar --> [foo.bar]
  • foo.2 --> [foo 2] (2 is an index in the sequence foo)
  • foo.:2 --> [foo 2] (2 is a key in the object foo)
  • foo:bar --> [foo:bar]

-1 can be used as an index to indicate a desire to append to a sequence.

This is slightly inconsistent with the syntax supported by our yaml-update step.

  • Indices are handled the way Helm does. e.g. foo.[2]
  • Negative indices not allowed at all -- appending to a sequence was not possible. (Technically it was, but only if you knew the length of the sequence, which effectively made it useless.)
  • No provision for escaped dots

These slightly different syntaxes were inevitably going to lead to confusion.

This PR updates the internal yaml library we use for editing YAML whilst preserving comments and formatting choices, and by extension the yaml-update step, to use the same syntax as tidwall/sjson and the new json-update step.

There are no breaking changes in this PR. The legacy syntax still works.

Since these changes open the possibility of using our internal yaml package, and by extension the yaml-update step, to append to a sequence (which it couldn't do before), the notion of using a map of key/value pairs as input to update operations is invalidated. If we can append to a sequence, order matters now...

So to enable all of the above, this PR also contains a small bit of refactoring to transition away from using maps as input to YAML update operations and toward using arrays of yaml.Update structs.

cc @fykaa

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit bed52c4
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67638b9daea2b90008390370
😎 Deploy Preview https://deploy-preview-3156.docs.kargo.io
📱 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 site configuration.

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 87.87879% with 12 lines in your changes missing coverage. Please review.

Project coverage is 51.37%. Comparing base (b75ef48) to head (bed52c4).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/yaml/update.go 89.65% 4 Missing and 2 partials ⚠️
internal/yaml/decode.go 25.00% 2 Missing and 1 partial ⚠️
internal/yaml/yaml.go 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3156      +/-   ##
==========================================
+ Coverage   51.27%   51.37%   +0.09%     
==========================================
  Files         285      285              
  Lines       25706    25763      +57     
==========================================
+ Hits        13182    13236      +54     
- Misses      11824    11825       +1     
- Partials      700      702       +2     

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

Signed-off-by: Kent Rancourt <[email protected]>
@krancour krancour marked this pull request as ready for review December 19, 2024 02:58
@krancour krancour requested review from a team as code owners December 19, 2024 02:58
Copy link
Contributor

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

💯

@krancour krancour added this pull request to the merge queue Dec 19, 2024
Merged via the queue into akuity:main with commit 124a3f1 Dec 19, 2024
21 checks passed
@krancour krancour deleted the krancour/yaml-pkg-improvement branch December 19, 2024 17:06
fykaa pushed a commit to fykaa/kargo that referenced this pull request Dec 20, 2024
fykaa pushed a commit to fykaa/kargo that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants