-
Notifications
You must be signed in to change notification settings - Fork 396
Support specifying custom values files #899
base: master
Are you sure you want to change the base?
Conversation
Helm has code for this here: https://github.com/helm/helm/blob/master/cmd/helm/install.go#L358 It would probably be a better approach to try move their code for it to k8s.io/helm/pkg/chartutil, and then use that here. For now I just copied out the necessary bits, because their version supports things like custom protocols, like values files from ftp or http, which I don't think we need. Though matching what helm supports is probably a good idea. TODO: I still need to add tests for this. Closes https://github.com/Azure/draft/issues/818 Closes https://github.com/Azure/draft/issues/856 Signed-off-by: William Stewart <zoidbergwill@gmail.com> Signed-off-by: William Stewart <zoidbergwill@gmail.com>
@@ -250,14 +251,67 @@ func loadArchive(ctx *Context) (err error) { | |||
return nil | |||
} | |||
|
|||
// Merges source and destination map, preferring values from the source map | |||
func mergeValues(dest map[string]interface{}, src map[string]interface{}) map[string]interface{} { |
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.
cc @bacongobbler, @michelleN
Are there any other projects you are aware of that re-implement the merging of value files from Helm?
Would it make sense to expose this in the Helm pkg
?
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.
Google's skaffold
, for example, just calls helm
binary directly and passes all valuesFiles
as arguments to it, i.e. it doesn't deal with merging them at all — but even if this is the first occurrence, I think it makes sense to expose this in the Helm.
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.
Should I make a PR for helm? Would chartutil
be the right package for it?
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'm pretty sure this already exists in pkg/chartutil
called MergeInto
: https://github.com/helm/helm/blob/403af2c3895e4fa0e04713d94135bf3438d9ce9c/pkg/chartutil/values.go#L97-L111
Probably would be good to use that instead
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.
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.
From what I could see, parts where supported by chartutil
already, but not other parts, will take a look at refining it, whether it's in Helm or here.
Related to https://github.com/Azure/draft/pull/899 Signed-off-by: William Stewart <zoidbergwill@gmail.com>
Any word when, or if, this will be merged? This is a must-have feature for my particular use case. |
Is there any way I can help get this pull request back alive? I can contribute to the code if necessary, but like @cws-credsimple, this is very important for my use case |
Helm has code for this here: https://github.com/helm/helm/blob/master/cmd/helm/install.go#L358
It would probably be a better approach to try move their code for it to
k8s.io/helm/pkg/chartutil, and then use that here. For now I just copied out
the necessary bits, because their version supports things like custom protocols,
like values files from ftp or http, which I don't think we need. Though
matching what helm supports is probably a better idea.
TODO: I still need to add tests for this.
Closes https://github.com/Azure/draft/issues/818
Closes https://github.com/Azure/draft/issues/856