-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix: helm parameters show effective values from inline values #25924
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
base: master
Are you sure you want to change the base?
fix: helm parameters show effective values from inline values #25924
Conversation
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
| if err := yaml.Unmarshal(q.Source.Helm.ValuesYAML(), &userVals); err == nil { | ||
| flattenValues(userVals, params, "") | ||
| } | ||
| } |
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.
Can we add a warning/log message when spec.source.helm.values is not a valid Helm values object?
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.
Added warning log when helm values parsing fails - will now log failed to parse helm values: <error> if the inline values YAML is malformed.
Mangaal
left a comment
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.
I have verified the changes locally and it is working as expected.
Could you please add some tests to cover this new logic? I have also left a small suggestion, please feel free to resolve it.
d7ef230 to
063d68a
Compare
Added 3 unit tests:
|
| if q.Source.Helm != nil && !q.Source.Helm.ValuesIsEmpty() { | ||
| var userVals map[string]any | ||
| if err := yaml.Unmarshal(q.Source.Helm.ValuesYAML(), &userVals); err != nil { | ||
| log.Warnf("failed to parse helm values: %v", err) | ||
| } else { | ||
| flattenValues(userVals, params, "") | ||
| } | ||
| } |
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 code unmarshals inline values into a map[string]any then calls flattenValues(userVals, params, "")
If the YAML is not a map at the document root (e.g., a top-level array), unmarshalling into map[string]any will yield nil (no error), and flattenValues will be called with nil, which will panic.
Also nested maps may sometimes have non-string key types (map[interface{}]interface{}) depending on the yaml parsing library
I recommend using the any type and change the code to something like this
| if q.Source.Helm != nil && !q.Source.Helm.ValuesIsEmpty() { | |
| var userVals map[string]any | |
| if err := yaml.Unmarshal(q.Source.Helm.ValuesYAML(), &userVals); err != nil { | |
| log.Warnf("failed to parse helm values: %v", err) | |
| } else { | |
| flattenValues(userVals, params, "") | |
| } | |
| } | |
| if q.Source.Helm != nil && !q.Source.Helm.ValuesIsEmpty() { | |
| var raw any | |
| if err := yaml.Unmarshal(q.Source.Helm.ValuesYAML(), &raw); err != nil { | |
| log.Warnf("failed to parse helm values: %v", err) | |
| } else { | |
| // flatten supports arbitrary root types and map[interface{}] keys | |
| flattenAnyValues(raw, params, "") | |
| } | |
| } |
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.
aaah, you are right! lemme fix this.
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.
Fixed in latest commit. Now using any type with type assertion:
var raw any
if err := yaml.Unmarshal(..., &raw); err != nil {
log.Warnf("failed to parse helm values: %v", err)
} else if userVals, ok := raw.(map[string]any); ok {
flattenValues(userVals, params, "")
} else if raw != nil {
log.Warnf("helm values is not a map, got %T", raw)
}Added edge case tests in Test_populateHelmAppDetails_WithInvalidValues:
top-level array should not panicscalar value should not panicmalformed yaml should not panic
Fixes argoproj#9213 PARAMETERS section showed chart defaults instead of actual values when using inline helm.values. Now merges user's inline values and parameter overrides into displayed parameters. - Use any type for unmarshal to handle non-map YAML (arrays, scalars) - Add warning log when values parsing fails or is not a map - Add unit tests for flattenValues, inline values merge, and edge cases Signed-off-by: Darshan Nij <[email protected]> Signed-off-by: nijdarshan <[email protected]>
b07d5dc to
e42a5b6
Compare



Summary
Fixes #9213
PARAMETERS section in UI showed chart defaults instead of actual values when using inline
helm.values. Now merges user's inline values and parameter overrides into displayed parameters.Changes
flattenValues()helper to convert nested YAML to dot-notationpopulateHelmAppDetails()to merge inline values and parameters into the params map before displayTest plan
values: replicaCount: 10- PARAMETERS now showsreplicaCount: 10instead of chart default1Signed-off-by: Darshan Nij [email protected]