Modernize template best practices regarding chomping and update some examples

[consideRatio]

Whitespace chomping are systematically done to the left in the templates output from helm create, and right whitespace chomping is reserved for when the initial content of a template file or defined template will follow. This PR will reflect that practice.

This PR also fixes smaller details such as to add YAML syntax highlighting in code blocks.

[hickeyma]

LGTM, thanks for the improvement to the doc @consideRatio.

I would like to get some more eyes on this to review before merging.

[consideRatio]

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?

[mattfarina]

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

[consideRatio]

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.

[consideRatio]

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 -}}

[consideRatio]

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 }}

[consideRatio]

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.

[consideRatio]

@mattfarina we discussed a section and I have now landed in a concrete idea: to remove the section which I question to be a best practice. This was done in dd2b5ff. I now consider this ready for merge in my mind.

[bridgetkromhout]

Hi, @mattfarina @hickeyma - thoughts on this PR? Should we have the merge conflict corrected so it can be merged, or are we past wanting this update? Thanks.

[consideRatio]

@bridgetkromhout thanks for the triage work! I rebased it in case its considered a positive change.

[bacongobbler]

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?

[consideRatio]

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 }}

[bacongobbler]

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?

[consideRatio]

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.

[bacongobbler]

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.

[consideRatio]

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.

[bacongobbler]

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.

[consideRatio]

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.

[consideRatio]

Thanks for the review @bacongobbler.

I fully appreciate your situation as a maintainer of an open source project and that this PR is a bit troublesome to review as it is all about opinionated changes rather than clear right and wrongs.

I've not fully complied with your requests but certainly adjusted based on your requests and tried motivating why I've been a bit stubborn.

---

I've also now made a new commit besides the previous changes - 8d74a71. I think it is an important addition to the documentation as there wasn't an example for the statement modified and it wasn't either a clear practice followed by helm create emitted templates for example.