Helmfile: Go templates `default` functionality is broken after version 0.33.0

[apetheriotis]

The following example (it's provided in the helmfile readme) works on version 0.33 but on version 0.34 and onwards:

helmfile.yaml

environments:
  production:
    values:
    - production.yaml

releases:
- name: myapp
  values:
  - values.yaml.gotmpl

production.yaml

domain: prod.example.com
releaseName: prod
values.yaml.gotmpl

domain: {{ .Environment.Values.domain | default "dev.example.com" }}

The above setup fails on version 0.34 and onwards with:

 map has no entry for key "domain"

when the domain value is missing from the environment file production.yaml

Probably the functionality of go templates is not the expected one when the key is missing from the values file.

[davidovich]

Yes, I have reproduced this, and it is related to template parsing defaults that have changed from missingkey=zero to missingkey=error. See #317 for discussion.

The change appeared while implementing #308.

But I would like @mumoshu and @sstarcher to chime in on this because they reflected on the problem, and concluded that failing on a missing key was a better outcome. If the new behavior is wanted, we would need to update this example in the README to correctly reflect this fact.

Meanwhile, as a workaround, you can add a default: key in the environments: dictionary referencing a default.yaml which defines domain:

environments:
  default:
     values:
        - default.yaml

[mumoshu]

Apologies I should have updated the doc about this. As @davidovich has kindly summarized, this is now a desired behavior.

The defaulting with the default func has possibility to hide the bug due to that you actually wanted to set an env value, but missed to do so. That's why I thought this should be a desired behavior. You should explicitly set the default value, in this case "dev.example.com", explicitly in your production.yaml.

Does this make sense? I'm happy to hear your opinions.

[apetheriotis]

Thank you both for the clarification and no worries about the docs.

You should explicitly set the default value, in this case "dev.example.com", explicitly in your production.yaml.

I think following the above approach results in having a lot of duplicated values . For example in our case with 30 apps and 6 environments we would need to copy the value replicas: 1 in 5 different files and only change it in one file (prod.yaml) replicas:2. Instead if we could default it, we would have to declare the value replicas: just once (only in prod.yaml).

[sstarcher]

@apetheriotis could you give an example of how you used it before so we can see what the best path forward will be?

[apetheriotis]

Sure. So:

Helmfiles structures

apps/
  notify-api/
     helmfile.yaml    
  ...           

helmfile of notify api looks similar this:

helmDefaults:
  tillerNamespace: dev

environments:
  dev:
    values:
    - ../../../values/environments/dev.yaml

releases:
- chart: foobar/ap-notify-api
  name: ap-notify-api
  namespace: dev
  values:
  - ../../../values/apps/notify-api/notify-api.yaml.gotmpl
  version: 1.2.3

(Notice here, that in version 0.37 I could template the version, namespace etc so I could have only one file for each app and not one file per app per environment )

Template structures

apps\
 notify-api\
   notify-api.yaml.gotmpl
 .... 

where notify-api.yaml.gotmpl is:

replicas: {{ .Environment.Values.eventApi.replicas | default 1 }}

dashboards:
  enabled: {{ .Environment.Values.notifyApi.dashboards.enabled | default false }}

Environment structures

dev.yaml

notifyApi:
  dashboards:
    enabled: true

prod.yaml

notifyApi:
  replicas: 10

^^ this is the point where the functionality of default is useful.

(apologies for the long example. but I hope it's more clear the output I expect)

[sstarcher]

I was going to recommend something along the lines of, but realized with the new error setup that is likely not possible.

{{ if .Environment.Values.eventApi.replicas }}
replicas: {{ .Environment.Values.eventApi.replicas }}
{{ endif }}

[sstarcher]

@mumoshu do we have any way to check if a key exists without trying to access it.

[mumoshu]

@sstarcher AFAIK, theres no way to do so.

We could probably add a template func that works like {{ exists ".Environment.Values.eventApi.replicas" }} but in my op it isn't very intuitive.

I think following the above approach results in having a lot of duplicated values

@apetheriotis This is definitely an issue that should be fixed somehow. I was thinking to allow "overlaying" environment values tmpl files like:

dev.yaml.gotmpl

{{ readFile "defaults.yaml "}}
---
notifyApi:
  dashboards:
    enabled: true

prod.yaml.gotmpl

{{ readFile "defaults.yaml "}}
---
notifyApi:
  replicas: 10

defaults.yaml

notifyApi:
  replicas: 1

But I'm not very sure if everyone thinks this intuitive. I personally prefer this way, as you can make each env values files self-contained - no need to consult each chart values template files to see what's the default value.

WDYT?

[sstarcher]

I would lean toward the exists which is essentially the opposite of helms required

[sstarcher]

Either that or something that works differently then default, which would allow us to do the lookup like exists, but to catch the error.

something like

{{ defaultValue ".Environment.Values.eventApi.replicas" 1 }}

Where is Environment.Values.eventApi.replicas is not set it would return a 1.

[mumoshu]

It seems to work. But in the meantime, how about reverting what we decided in #317, or hiding it behind a kind of feature flag?

I think we need some time to settle on a single solution, and also forcing users to rewrite dozens of values.yaml.gotmpl files isn't what we'd want?

[sstarcher]

I'm still against reverting it due to it allowing random nulls through. Which gives a user a false sense of security. After upgrading to 0.37.0 I found no less then a dozen instances where things were misconfigured by users in helmfile due to null entries being silently ignored.

I can more agree to a feature flag until we get it hashed out.

[sstarcher]

The preferred option would be to support both capabilities a strict mode and a default. As long as we support both I'm fine, but I believe from a usability standpoint and a error resolution standpoint strict mode makes the most sense to be the default.

[mumoshu]

Makes sense! Just to be sure,

and a default

Do you mean either {{ defaultValue ... }} thing, or overlays in values.yaml.gotmpl I suggested above, or both?

[sstarcher]

I'm not a fan of the overlay. I would think something like defaultValue or some other name would be better.

[apetheriotis]

maybe defaultValueIfKeyAbsent or too big ?

[sstarcher]

onNull ?

{{ onNull ".Environment.Values.eventApi.replicas" 1 }}
{{ defaultValue ".Environment.Values.eventApi.replicas" 1 }}
{{ defaultValueIfKeyAbsent ".Environment.Values.eventApi.replicas" 1 }}

[mumoshu]

How about {{ .Environment.Values | getOrNil "eventApi.replicas" | default 1 }}? A side-effect of thjis is that it also allows using hyphens in keys which I saw in another issue, and allows digging things other than .Environment too.

[sstarcher]

I like it

[mumoshu]

@apetheriotis @sstarcher Hey! Thanks for awesome feedbacks.

I've added the getOrNil func and documented it in https://github.com/roboll/helmfile/blob/master/docs/writing-helmfile.md#missing-keys-and-default-values. Would it work for you?

@apetheriotis Do you prefer the fail-on-missing-key behavior to be configurable? Or are you ok with just rewriting the way you implement default values, and benefit from helmfile's new ability to report any missing env values?

[apetheriotis]

Thank you for fixing that !

Personally I don't prefer having another flag. I'd say as it is now it's good enough 👍

[mumoshu]

@apetheriotis @sstarcher I'm going to deprecate getOrDefault and enhance get as described in #465