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.

Sign up for GitHub

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

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

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c65ac3
Update template best practices regarding chomping
consideRatio Oct 13, 2020
8693c54
Template best practices: removed edge case idea
consideRatio Oct 29, 2020
1747d1a
Template best practices: warned about common syntax error
consideRatio May 9, 2021
d89e4a8
docs: fix syntax error in example
consideRatio May 9, 2021
0b81002
Adjust based on requested changes
consideRatio May 9, 2021
8d74a71
Template best practices: declared template indentation practice
consideRatio May 9, 2021
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
83 changes: 50 additions & 33 deletions content/en/docs/chart_best_practices/templates.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,62 +31,73 @@ For that reason, _all defined template names should be namespaced._
Correct:

```yaml
{{- define "nginx.fullname" }}
{{/* ... */}}
{{ end -}}
{{- 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 •
edited
Loading

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.

# ...
Copy link
Member

Choose a reason for hiding this comment

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

It's not totally clear here why you swapped out the Go template comment syntax with YAML comments. The former will be removed from the rendered output, while the latter will not. It also introduces the user to the text/template syntax.

https://golang.org/pkg/text/template/#hdr-Actions

Can you please revert this as well as any other similar changes in this PR?

Copy link
Author

@consideRatio consideRatio May 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'll switch back, but my intent was the following:

  • I felt it increased readability, to a large extent because of syntax highlighting
  • Because it felt like it communicated better that it was placeholder content
  • Because using template comments, it would be easy to make it look complicated as whitespace chomping would either be involved or not involved and break common practices

Either...

For that reason, all defined template names should be namespaced.

Correct:

{{- define "nginx.fullname" -}}
{{/* ... */}}
{{- end }}

Incorrect:

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

Or...

For that reason, all defined template names should be namespaced.

Correct:

{{- define "nginx.fullname" -}}
# ...
{{- end }}

Incorrect:

{{- define "fullname" -}}
# ...
{{- 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 }}
{{ print "foo" }}
{{- print "bar" -}}
```

Incorrect:
```
{{.foo}}
{{print "foo"}}
{{-print "bar"-}}
```yaml
{{- /* Conditionally create service account */}}
{{- if .Values.serviceAccount.create }}
Copy link
Member

Choose a reason for hiding this comment

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

This will error out if serviceAccount is a null value. I would refrain from using nested values in examples. Especially since the ONLY thing this example is trying to convey is "template formatting".

The original examples were just fine IMO. Why are we changing this?

Copy link
Author

@consideRatio consideRatio May 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Updated again, there is still a small opinionated tweak making the example two lines instead of three and making the third line not include a right whitespace chomp.

apiVersion: v1
# ...
{{- end }}
```

Templates should chomp whitespace where possible:
Incorrect:

```yaml
foo:
{{- range .Values.items }}
{{ . }}
{{ end -}}
{{-/* This comment is even a syntax error */}}
Copy link
Member

Choose a reason for hiding this comment

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

It's probably best to not overload the user with more than one topic at one time. Please keep this focused on the content that is trying to be conveyed. To clarify, here we only want to point out that "Template directives should have whitespace after the opening braces and before the closing braces". Anything more than that can lead to confusion for the reader.

I would also remove the dashes from this example. Again we're only trying to introduce the bare minimum here to keep the content clear and concise. Whitespace chomp syntax is introduced further in the article.

Copy link
Author

Choose a reason for hiding this comment

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

It's probably best to not overload the user with more than one topic at one time.

I agree - example minimized to a single line.

{{-if .Values.serviceAccount.create}}
apiVersion: v1
# ...
{{-end}}
```

Blocks (such as control structures) may be indented to indicate flow of the
template code.
Templates should chomp unnecessary whitespace. A good practice is to
systematically _chomp left_, but also _chomp right_ for initial content in a
template file or defined template.

```yaml
myList:
{{- range .Values.elements }}
- {{ . }
{{- end }}
```
{{ if $foo -}}
{{- with .Bar }}Hello{{ end -}}
{{- end -}}

```yaml
{{- define "nginx.selectorLabels" -}}
app.kubernetes.io/name: {{ include "nginx.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}
```

However, since YAML is a whitespace-oriented language, it is often not possible
for code indentation to follow that convention.
```yaml
{{- if .Values.serviceAccount.create -}}
apiVersion: v1
# ...
{{- end }}
```

## Whitespace in Generated Templates

Expand Down Expand Up @@ -147,17 +158,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,9 +184,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 }}

```

Expand Down