GoogleContainerTools / GoogleContainerTools/skaffold

Support default values in all templateable helm fields

Open
#9,062 4 comments 2 reactions 0 assignees View on GitHub
area/templating kind/friction priority/p2
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.