Conversation
blampe
added
the
impact/no-changelog-required
Does the PR have any schema changes?Looking good! No breaking changes found. |
| want: map[string]interface{}{ | ||
| "configuration": map[string]any{ | ||
| "backupStorageLocation": []map[string]any{}, | ||
| "backupStorageLocation": []any{}, |
There was a problem hiding this comment.
A subtle (probably inconsequential) difference between the previous MergeValues and upstream's.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3010 +/- ##
=======================================
Coverage 32.29% 32.29%
=======================================
Files 69 69
Lines 8947 8963 +16
=======================================
+ Hits 2889 2895 +6
- Misses 5791 5796 +5
- Partials 267 272 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| cmd.addChartPathOptionsFlags() | ||
| } | ||
|
|
||
| func (cmd *TemplateOrInstallCommand) addValueOptionsFlags() { |
There was a problem hiding this comment.
I would still keep addValueOptionsFlags to set defaults on Values and to further resemble:
https://github.com/helm/helm/blob/14d0c13e9eefff5b4a1b511cf50643529692ec94/cmd/helm/flags.go#L45-L51
| // readAssets converts Pulumi values to Helm values, hydrating Asset values | ||
| // along the way. | ||
| func readAssets(p getter.Providers, a map[string]interface{}) (map[string]interface{}, error) { |
There was a problem hiding this comment.
I feel like marshalValues is a better name because it isn't just dealing with assets; imagine that resource references were marshaled in some way.
| Values: tt.values, | ||
| } | ||
| p := getter.All(cli.New()) | ||
| opts, cleanup, err := readValues(p, tt.values, tt.valuesFiles) |
There was a problem hiding this comment.
Can you use getter.Providers{} for p?
|
On one hand, this approach allows us to leverage more of the Helm library, specifically related to loading values files. On the other hand, the need to write out the adhoc values to a file seems a bit contrived. Let's vote on it, in description. |
|
@blampe would you agree to closing this now? |
2 participants
Followup to my comment here, this removes our custom
ValueOptsin favor of upstream'svalue.Optionsto handle merging.Upstream expects to work with files on disk, so we fetch any assets in our provided values and then write them to disk temporarily.