-
Notifications
You must be signed in to change notification settings - Fork 67
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
render-helm-chart function changes #747
render-helm-chart function changes #747
Conversation
f177db3
to
77bbc56
Compare
77bbc56
to
c2d46b9
Compare
...der-helm-chart/third_party/sigs.k8s.io/kustomize/api/builtins/HelmChartInflationGenerator.go
Outdated
Show resolved
Hide resolved
...der-helm-chart/third_party/sigs.k8s.io/kustomize/api/builtins/HelmChartInflationGenerator.go
Outdated
Show resolved
Hide resolved
...der-helm-chart/third_party/sigs.k8s.io/kustomize/api/builtins/HelmChartInflationGenerator.go
Outdated
Show resolved
Hide resolved
...der-helm-chart/third_party/sigs.k8s.io/kustomize/api/builtins/HelmChartInflationGenerator.go
Show resolved
Hide resolved
1dbb935
to
1390af0
Compare
1390af0
to
1e7e56c
Compare
cc @yuwenma |
`helmCharts` | An array of helm chart parameters | ||
`chartArgs` | Arguments that describe the chart being rendered. | ||
`templateOptions` | A collection of fields that map to flag options of `helm template`. | ||
`chartHome` | A filepath to a directory of charts. The function will look for the chart in this local directory before attempting to pull the chart from a specified repo. Defaults to "tmp/charts". When run in a container, this path MUST have the prefix "tmp/". | tmp/charts |
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.
Same comment here. tmp/
or /tmp
?
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.
For chartHome
, the default is tmp/
. IIUC when run in a container, the code is running at the root so it becomes the same thing as /tmp
. However for chartHome
it is important to distinguish, because when run locally (i.e. exec
) it will look for files in tmp/charts
in the current directory by default, not /tmp/charts
. I have updated the containerized valuesFiles
examples to use /tmp
to be explicit, but chartHome
is tmp
and not /tmp
intentionally.
...der-helm-chart/third_party/sigs.k8s.io/kustomize/api/builtins/HelmChartInflationGenerator.go
Show resolved
Hide resolved
return path, ioutil.WriteFile(path, b, 0644) | ||
// use a hash of the provided path to generate a unique, valid filename | ||
hash := md5.Sum([]byte(path)) | ||
newPath := filepath.Join(p.tmpDir, p.Name+"-kustomize-values-"+hex.EncodeToString(hash[:])+".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.
Is hex.EncodeToString(md5.Sum(input))
a common practice in kustomize?
I guess hex.EncodeToString(hash[:])
can be simplified to hex.EncodeToString(hash)
, right?
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.
It is not something we do in kustomize, but the hasher we have in kustomize hashes an entire RNode rather than a string, the code is here: https://github.com/kubernetes-sigs/kustomize/blob/master/api/hasher/hasher.go
Alternatives here could be to use the kustomize's hasher and do something like hasher.SortArrayAndComputeHash([]string{path})
- but we are not using this anywhere in kustomize so I am unsure how reliable it is. Or try something like the kubernetes deep hasher object.
However I prefer to use hex.EncodeToString(md5.Sum(input))
because it seemed like the simplest thing to do here.
I guess hex.EncodeToString(hash[:]) can be simplified to hex.EncodeToString(hash), right?
The latter doesn't compile, because hash
is type [16]byte
but hash[:]
is type []byte
.
RenderHelmChart
functionConfig (helmChartInflationGenerator: Support multiple helm values files kubernetes-sigs/kustomize#4219)repo
field optional to support authentication with private repositories (Support helm authentication for helm chart repositories. kubernetes-sigs/kustomize#4335)helm template
, such asapi-versions
,name-template
, andskip-tests
.EDIT: Removing
create-namespace
andskip-crds
because I'd misunderstood what they do; I don't think they will provide value here as IIUC they involve communication with the cluster.This is a breaking change, and will require a version bump.