GoogleContainerTools / GoogleContainerTools/skaffold
Support default values in all templateable helm fields
- Dominant language
- Go
- Stars
- 15.9k
- Forks
- 1.7k
- Avg merge
- 3d 9h
- Merged PRs (30d)
- 10
Description
Using the helm deployer, some fields are available for templating.
These include `deploy.helm.releases[].name` and `deploy.helm.releases[].setValueTemplates`. For some of those fields, it is necessary that if a value is set, the value is not empty (e.g. but not limited to the `deploy.helm.releases[].name`).
This is currently achieved by [util.ExpandEnvTemplateOrFail(...)](https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/util/env_template.go#L52) which set's `missingkey=error` introduced for #5072 .
While I very much agree that failing fast for an accidentally empty value is important, this has the consequence that trying to use the default function like `{{ default "foo" .NAME }}` is also not possible if `.NAME` is not defined, which in my opinion invalidates the usage of default.
### Expected behavior
Fields which support templating should IMHO support the same templating language and logic.
### Actual behavior
Some fields support templating defaults, some don't.
### Information
- Skaffold version: 2.6.0
- Operating system: Ubuntu 20.04
- Installed via: skaffold.dev
- Contents of skaffold.yaml:
```yaml
apiVersion: skaffold/v4beta6
kind: Config
metadata:
name: skaffold
deploy:
helm:
releases:
- name: '{{ default "foo" .NAME }}'
chartPath: helm
setValueTemplates:
image.tag: '{{ default "tag" .TAG }}'
```
### Steps to reproduce the behavior
`skaffold deploy` or `skaffold render` both fail with:
```
$ skaffold deploy
Starting deploy...
cannot expand release name "{{ default \"foo\" .NAME }}": template: envTemplate:1:17: executing "envTemplate" at <.NAME>: map has no entry for key "NAME"
```
### Proposed solution
For fields in helm currently using `util.ExpandEnvTemplateOrFail(...)`, switch to the existing `util.ExpandEnvTemplate(...)` (which does not error on a missing key) and handle an empty string returned as an error after templating. It may be a good idea to centralize the templating of those fields into `pkg/skaffold/helm/util.go` to ensure consistent behaviour between [render](https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/render/renderer/helm/helm.go#L146) and [deploy](https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/helm/helm.go#L261), but I don't know the codebase well, so I'm very open to suggestions here.
If this is a viable Path forward, I'd be interested in starting a PR. Feel free to let me know any other feedback as well.
Greets,
Christoph
Contributor guide
Assessment
This issue has not been assessed yet.