Fix the Helm trick that we use to differentiate between 0 and an empty value

[inteon]

Closes #6712

This trick was introduced in #5860 and #6248.
The root issue is that {{ if "" }} and {{ if 0 }} both return false. The trick was introduced to fix this issue.

The trick uses the quote function to differentiate between a empty string and 0 or "0".
The result from quote can be compared with (quote "") to detect empty strings.

However, the fix is not complete. In case the value is empty/ null, the result from quote is an empty string not (quote "").

The final solution that fixes the original and the new problem is to:

1. use (quote value): yields these strings "0", "" and an empty string
2. compare the strings with "" or an empty string

before:

$ helm template . --set global.revisionHistoryLimit=0 | grep revisionHistoryLimit
  revisionHistoryLimit: 0
  revisionHistoryLimit: 0
  revisionHistoryLimit: 0
$ helm template . --set global.revisionHistoryLimit=5 | grep revisionHistoryLimit
  revisionHistoryLimit: 5
  revisionHistoryLimit: 5
  revisionHistoryLimit: 5
$ helm template  . --set global.revisionHistoryLimit= | grep revisionHistoryLimit
$ helm template  . | grep revisionHistoryLimit
  revisionHistoryLimit: 
  revisionHistoryLimit: 
  revisionHistoryLimit: 

after:

$ helm template . --set global.revisionHistoryLimit=0 | grep revisionHistoryLimit
  revisionHistoryLimit: 0
  revisionHistoryLimit: 0
  revisionHistoryLimit: 0
$ helm template . --set global.revisionHistoryLimit=5 | grep revisionHistoryLimit
  revisionHistoryLimit: 5
  revisionHistoryLimit: 5
  revisionHistoryLimit: 5
$ helm template  . --set global.revisionHistoryLimit= | grep revisionHistoryLimit
$ helm template  . | grep revisionHistoryLimit

Kind

/kind bug

Release Note

Helm: Fix a bug in the logic that differentiates between 0 and an empty value.

[wallrj]

I see lots of discussion of this problem in:

• Wrong interpretation of Zero in templates files when having a default value helm/helm#3164
• Clarification: nil vs empty string Masterminds/sprig#53

And the work arounds mentioned in the comments of those issues are:

• Wrong interpretation of Zero in templates files when having a default value helm/helm#3164 (comment) by @aczajkowski

if has (kindOf $value) (list "int" "int64" "float" "float64")

• Clarification: nil vs empty string Masterminds/sprig#53 (comment) by @jkroepke (one of our recent contributors)

if kindIs "invalid" $value

Neither of those issues seems to mention the hasKey $parent "key" trick which is already used in our chart, but I wonder why.

Happy to approve this PR if you are sure your's is the best fix and clearly it does fix the problem - thanks for showing all your testing - but also please check out those issues and look at the other tricks used already in our chart and explain why this new trick is necessary here.

[wallrj]

Elsewhere in this chart we use if hasKey .Values.global "revisionHistoryLimit:

cert-manager/deploy/charts/cert-manager/templates/webhook-deployment.yaml

Lines 47 to 49 in 6a9c402

	{{- if hasKey .Values.webhook "automountServiceAccountToken" }}
	automountServiceAccountToken: {{ .Values.webhook.automountServiceAccountToken }}
	{{- end }}

Would that technique work here?

[inteon]

@wallrj we also want to skip the property when the value is not set or set to an empty value in the values.yaml file or using a command line flag. This solution would also set the property when the value is just an empty string.

[wallrj]

If the user has used --set global.revisionHistoryLimit= then revisionHistoryLimit: should appear in the rendered manifests, since that is what they have asked for.

[inteon]

How would you unset the value from the command line then?

[jkroepke]

Neither of those issues seems to mention the hasKey $parent "key" trick which is already used in our chart, but I wonder why.

I guess it wont work, if the default already contains the key and as end user I want to override the key with nil. Technically, the key is present with an nil value.

[bodgit]

I used this variant in a recent PR to another chart:

  {{- if or (kindIs "float64" .Values.revisionHistoryLimit) (kindIs "int64" .Values.revisionHistoryLimit) }}
  revisionHistoryLimit: {{ .Values.revisionHistoryLimit | int64 }}
  {{- end }}

which I guess is a variant of the if has (kindOf $value) (list "int" "int64" "float" "float64") trick. I never saw the int or float types pop up though. Annoyingly the type changes depending on how/where the value is set, (--set vs YAML).

[inteon]

I used this variant in a recent PR to another chart:

  {{- if or (kindIs "float64" .Values.revisionHistoryLimit) (kindIs "int64" .Values.revisionHistoryLimit) }}
  revisionHistoryLimit: {{ .Values.revisionHistoryLimit | int64 }}
  {{- end }}

which I guess is a variant of the if has (kindOf $value) (list "int" "int64" "float" "float64") trick. I never saw the int or float types pop up though. Annoyingly the type changes depending on how/where the value is set, (--set vs YAML).

Thanks for providing this alternative solution.
I think I would still prefer the quote trick, because that can also handle string values (eg. set "0" in the values.yaml file).
The KindIs solution uses the implicit type conversion that Helm does when it receives an integer, I and would prefer not to rely on that.
WDYT?

@wallrj I also just realised that it is not possible to put this logic in a template, because it is the argument of an if statement. Templates can only be used to template part of the output yaml. It cannot be used to define boolean functions.

[wallrj]

Thanks for updating the PR description with the new variant of the trick (which no longer uses squot).
And thanks for explaining to me in person why those other work arounds do not work here.

Please add comments above the tricks in the code, explaining why the if logic is written in this tricky way.

[wallrj]

Please add a comment above all these explaining why it is written this way and link to the upstream bugs.

[wallrj]

I see that you already added the comments.

/lgtm

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [wallrj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[inteon]

/cherrypick release-1.14

[jetstack-bot]

@inteon: new pull request created: #6729

In response to this:

/cherrypick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.