Support default values in all templateable helm fields

[girstenbrei]

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(...) 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:

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 and deploy, 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

[ericzzzzzzz]

Hi @girstenbrei the default function issue should be fixed in #8872, the release includes that change should happen soon. More importantly, Skaffold will be supporting all templates from sprig thanks brian for making the change #9005

[ericzzzzzzz]

oh sorry .. i misunderstood the problem, thought all places are using missingkey=invalid now, it turns out that only tagging phase uses missingkey=invalid strategy. We need to use the same method for templateExpanding.

[briantopping]

I'd also like to see templating used everywhere for starters, then adding the default key in the dot context for every script, changing depending on it's usage. This is a really spectacular idea!

I took a look at adding templating everywhere prior to creating an issue for it, but was unable to see a clear path to doing so. My strategy may have been a dead end. It wasn't about parsing and executing the templates (although that should happen in a single pass and the parses should be cached for execution speed), it's about managing the dynamic context and executing the template when it is used. The templates simply cannot be executed earlier than they are used because they will by nature depend on values generated elsewhere in the lifecycle(s).

@ericzzzzzzz, is there some place that the original static (configuration) context as well as current dynamic context can be accessed in a read-only manner?

[girstenbrei]

@ericzzzzzzz Thanks for the hint, I missed #8872 . The solution should be somewhat similar to it, I guess.

As you already created #9063 , I would defer the discussion which fields to template and what information is available to template with. For this issue, I'd like to focus on changing how templating evaluates to allow using the default function. IMHO this is a separate thing to solve and will enhance future templating possibilities without hindering efforts like #9063 .