chore: fix generic map conversion #1838
Conversation
|
This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID. Online Installer: Airgap Installer (may take a few minutes before the airgap bundle is built): Happy debugging! |
| for i, v := range in { | ||
| result[i] = cleanUpInterfaceMap(v) | ||
| func cleanUpGenericMap(m map[string]interface{}) (map[string]interface{}, error) { | ||
| b, err := yaml.Marshal(m) |
There was a problem hiding this comment.
i think because k8syaml cannot marshal a map[interface{}]interface{}. i will test and add a comment
There was a problem hiding this comment.
i added comments to clarify
| "float": 1.5, | ||
| "bool": true, | ||
| "array": []map[string]interface{}{ | ||
| { | ||
| "array": []interface{}{ |
There was a problem hiding this comment.
think helm will accept an []interface{}?
There was a problem hiding this comment.
i assume since its passing tests...
There was a problem hiding this comment.
our e2e tests never caught this bug, which is why these unit tests were introduced. so i wouldn't be so confident...
There was a problem hiding this comment.
if want did not change at all in these unit tests, i would be more confident in the change.
There was a problem hiding this comment.
emosbaugh
force-pushed
the
emosbaugh/20250206/generic-map-conversion
branch
from
f596145 to
7e18a4e
Compare
| req.NoError(err, "cleanUpGenericMap failed") | ||
| req.Equal(tt.want, out) | ||
|
|
||
| // ultimately helm calls k8syaml.Marshal so we must make sure that the output is compatible |
There was a problem hiding this comment.
did you verify (even if manually) that this would've caught the issue fixed by #1834?
There was a problem hiding this comment.
How can I verify? Can you provide reproduction steps? They are not in the original fix.
What this PR does / why we need it:
Makes map conversion more resilient. No longer have to add new case statements for each type.
Which issue(s) this PR fixes:
Does this PR require a test?
Does this PR require a release note?
Does this PR require documentation?