Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize template best practices regarding chomping and update some examples #892

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 135 additions & 30 deletions content/en/docs/chart_best_practices/templates.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,65 +31,149 @@ For that reason, _all defined template names should be namespaced._
Correct:

```yaml
{{- define "nginx.fullname" }}
{{- define "nginx.fullname" -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO I would remove whitespace chomping syntax until after it's been introduced to the reader. Introducing it before the user knows what it's supposed to do will only lead to questions/confusion.

Copy link
Author

@consideRatio consideRatio May 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to convey a best practice in this article, and leading by examples not following such practices only to correct them in the end wouldn't be very effective I believe - and could potentially be counter-effective.

As an example on why I believe that, consider the science on how to debunk a misconception/myth. It has been found that it is bad to first declare the misconception/myth. Instead one should first declare how it really is, then warn that a misconception/myth is about to be mentioned, mention the myth/misconception, and finally explain why it was a misconception/myth.

Similarly, I don't think we should lead with examples breaking the recommended practice only to later suggest how the practice should be. I know comparing this to debunking myths isn't correct, and I know there is value to increase the complexity step by step. With that said based on my experience as a teacher, from what I've learned from research, and using this documentation as a Helm chart developer, I believe the biggest concern is to be inconsistent.

So my suggestion would be to either lead with the whitespace chomping section so it can be introduced directly, or to be consistent in the whitespace chomping practices until the practices are introduced.

{{/* ... */}}
{{ end -}}
{{- end }}
```

Incorrect:

```yaml
{{- define "fullname" -}}
{{/* ... */}}
{{ end -}}
{{- end }}
```

It is highly recommended that new charts are created via `helm create` command
as the template names are automatically defined as per this best practice.

## Formatting Templates

Templates should be indented using _two spaces_ (never tabs).

Template directives should have whitespace after the opening braces and before
Template directives should have whitespace after the opening braces and before
the closing braces:

Correct:
```
{{ .foo }}

```yaml
{{ print "foo" }}
{{- print "bar" -}}
{{- print "bar" }}
```

Incorrect:
```
{{.foo}}

```yaml
{{print "foo"}}
{{-print "bar"-}}
{{-print "bar"}}
```

Templates should chomp whitespace where possible:
Templates should chomp unnecessary whitespace. A good practice is to
systematically _chomp left_, but also _chomp right_ for initial content in a
defined template or a template file.

```yaml
foo:
{{- range .Values.items }}
{{ . }}
{{ end -}}
myList:
{{- range .Values.elements }}
- {{ . }}
{{- end }}
```

Blocks (such as control structures) may be indented to indicate flow of the
template code.
```yaml
{{- define "nginx.selectorLabels" -}}
app.kubernetes.io/name: {{ include "nginx.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}
```

```yaml
{{- /* Initial content of a template file */ -}}
apiVersion: v1
```
{{ if $foo -}}
{{- with .Bar }}Hello{{ end -}}
{{- end -}}

Template logic should align indentation with associated content.

Correct:

```yaml
metadata:
annotations:
{{- if .Values.foo }}
foo: true
{{- end }}
bar: true
{{- with .Values.extraAnnotations }}
{{- . | toYaml | nindent 4 }}
{{- end }}
```

However, since YAML is a whitespace-oriented language, it is often not possible
for code indentation to follow that convention.
Incorrect:

```yaml
metadata:
annotations:
{{- if .Values.foo }}
foo: true
{{- end }}
bar: true
{{- with .Values.extraAnnotations }}
{{- . | toYaml | indent 4 }}
{{- end }}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm this is a nonsensical example that would generate bad output. I think personally this is a bad practice except for a very limited amount of situations.

I'd suggest to consider removing this section, leaving it out is simply to not actively not recommend or condemn doing so - that i think makes the most sense for this practice of indenting blocks. I would only consider this relevant for helpers with massive logic that doesnt render output but simply modifies variables, but even then im sceptical about deviating from the practice of not doing it.

Should i update the example to make somr sense at all, which may end up being such edge case, or shall i remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is useful. The use of indentation.

The old example used both if and with which is a bad example, too. What about using something like...

{{- with .Values.ingress.annotations }}
annotations:
  {{- toYaml . | nindent 4 }}
{{- end }}

This is right from generated templates done by helm create

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the docs point where this example resides, was orginally to suggest this kind of practice was acceptable, which i personally don't find it to be:

annotations:
{{- if something }}
  {{- if something-else }}
    {{- if something-else-again }}
  my.silly.org/annotation: "really silly"
    {{- end }}
  {{- end }}
{{- end }}

I think one should always do this instead.

annotations:
  {{- if something }}
  {{- if something-else }}
  {{- if something-else-again }}
  my.silly.org/annotation: "really silly"
  {{- end }}
  {{- end }}
  {{- end }}

I don't know if there is clear consensus on this kind of practice in the Helm docs anywhere, except for the _helpers.tpl file is mostly following the latter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recommendation is that this entire example is removed.

Blocks (such as control structures) may be indented to indicate flow of the
template code.

{{ if $foo -}}
  {{- with .Bar }}Hello{{ end -}}
{{- end -}}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, replaced with an example that one should NOT try to indent templates based on nested template logic, but stay on the indentation level based on the YAML.

DO THIS

annotations:
  {{- if something }}
  {{- if something-else }}
  {{- if something-else-again }}
  my.silly.org/annotation: "really silly"
  {{- end }}
  {{- end }}
  {{- end }}

DO NOT DO THIS:

annotations:
  {{- if something }}
    {{- if something-else }}
      {{- if something-else-again }}
  my.silly.org/annotation: "really silly"
      {{- end }}
    {{- end }}
  {{- end }}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I conclude that the most reasonable idea is to remove this section. A clear motivation for this is that it is currently having a comment after it saying

Blocks (such as control structures) may be indented to indicate flow of the
template code.

However, since YAML is a whitespace-oriented language, it is often not possible for code indentation to follow that convention.

See dd2b5ff for the change resolving this discussion in my mind - it removes this section. In my mind, it is a suggestion of a best practice I don't agree with, and, which only is relevant in very rare situations.

```

To align templates logic with associated content, the `nindent` function
together with left whitespace chomping is often required. `nindent` works
exactly like `indent` but prefixes a new line that can compensate for our left
whitespace chomping.

Correct:

```yaml
metadata:
annotations:
foo: true
{{- with .Values.extraAnnotations }}
{{- . | toYaml | nindent 4 }}
{{- end }}
```

Incorrect:

```yaml
metadata:
annotations:
foo: true
{{- with .Values.extraAnnotations }}
{{- . | toYaml | indent 4 }}
{{- end }}
```

## Whitespace in Generated Templates

Generated content should be indented with increments of _two spaces_.

Correct:

```yaml
apiVersion: batch/v1
kind: Job
metadata:
name: example
labels:
first: first
second: second
```

Incorrect:

```yaml
apiVersion: batch/v1
kind: Job
metadata:
name: example
labels:
first: first
second: second
```

It is preferable to keep the amount of whitespace in generated templates to a
minimum. In particular, numerous blank lines should not appear adjacent to each
other. But occasional empty lines (particularly between logical sections) is
Expand Down Expand Up @@ -147,17 +231,23 @@ metadata:
Both YAML and Helm Templates have comment markers.

YAML comments:

```yaml
# This is a comment
type: sprocket
foo: bar # This is a comment, bar isn't
```

Template Comments:
Helm template comments:

```yaml
{{- /* This is a comment (with chomping to the right). */ -}}
type: frobnitz

{{- /*
This is a comment.
This is a multiline comment (with no chomping to the right).
*/}}
type: frobnitz
foo: bar
```

Template comments should be used when documenting features of a template, such
Expand All @@ -167,10 +257,9 @@ as explaining a defined template:
{{- /*
mychart.shortname provides a 6 char truncated version of the release name.
*/}}
{{ define "mychart.shortname" -}}
{{- define "mychart.shortname" -}}
{{ .Release.Name | trunc 6 }}
{{- end -}}

{{- end }}
```

Inside of templates, YAML comments may be used when it is useful for Helm users
Expand All @@ -184,6 +273,22 @@ memory: {{ .Values.maxMem | quote }}
The comment above is visible when the user runs `helm install --debug`, while
comments specified in `{{- /* */}}` sections are not.

When using template comments, watch out for a common syntax error.

Correct:

```yaml
{{- /* Correct spacing with whitespace chomping*/ -}}
{{/* Correct spacing without whitespace chomping */}}
```

Incorrect:

```yaml
{{-/* Incorrect spacing with whitespace chomping */-}}
{{ /* Incorrect spacing without whitespace chomping */ }}
```

## Use of JSON in Templates and Template Output

YAML is a superset of JSON. In some cases, using a JSON syntax can be more
Expand Down