-
Notifications
You must be signed in to change notification settings - Fork 283
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: preserve formatting and comments when updating Helm values file #1039
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1039 +/- ##
==========================================
- Coverage 62.59% 62.49% -0.10%
==========================================
Files 15 15
Lines 2259 2293 +34
==========================================
+ Hits 1414 1433 +19
- Misses 755 764 +9
- Partials 90 96 +6 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yevhen Kyriukha <[email protected]>
yaml, err = marshalParamsOverride(&app, originalData) | ||
require.NoError(t, err) | ||
assert.NotEmpty(t, yaml) | ||
assert.Equal(t, strings.TrimSpace(strings.ReplaceAll(expected, "\t", " ")), strings.TrimSpace(string(yaml))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this part of the test is removed? I think we should keep any existing test as much as we can, and add new tests for introduced new features.
The above test verifies that
- the updater can infer the correct param name (whether to interpret dotted names as nested or flat ones) based on the existing format in use.
- any added new parameters will not corrupt or interfere with any existing parameters.
yaml, err = marshalParamsOverride(&app, originalData) | ||
require.NoError(t, err) | ||
assert.NotEmpty(t, yaml) | ||
assert.Equal(t, strings.TrimSpace(strings.ReplaceAll(expected, "\t", " ")), strings.TrimSpace(string(yaml))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this part of the test is removed? I think we should keep any existing test as much as we can, and add new tests for introduced new features.
The above test verifies that
- the updater can infer the correct param name (whether to interpret dotted names as nested or flat ones) based on the existing format in use.
- any added new parameters will not corrupt or interfere with any existing parameters.
// Check if the full key exists | ||
if idx, found := findHelmValuesKey(*currentValues, key); found { | ||
(*currentValues)[idx].Value = value | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current behaviro is, if the full key (e.g., images.nginx.name
) already exists in the target helm file, take this existing key and update its value. Otherwise, assume the users want nested parameter names. I think this should be perserved when the annotation value helm.image-name
and helm.image-tag
are not explicitly quoted.
Even when the 2 annotation values are explicitly quoted, it's still worth considering whether we should honor the existing key format (flat dotted or nested) over the application annotation.
Description
This PR enhances the Helm values file update functionality by preserving the original formatting and comments. Previously, updating the file would overwrite manual formatting and comments, which could be problematic for users who rely on these customizations for clarity and maintenance. By retaining inline comments and specific formatting, this change improves the user experience and reduces potential confusion after automated updates.
Additionally, this update refines how
image-name
andimage-tag
keys are handled:By default, key path in
image-name
andimage-tag
creates a nested structure. For example:Produces:
If a user wants to update Helm values that contain dots, the key path must be quoted (either with double or single quotes):
This will correctly produce:
These changes ensure greater flexibility and correctness in handling Helm values updates while maintaining user-defined formatting.