helm: Improve the Helm chart documentation. #16469

bmcustodio · 2021-06-08T08:35:38Z

helm-docs has an issue that causes some of our fields to have wrong documentation associated with them (e.g. containing documentation belonging to other, commented-out fields). This PR attempts to fix them by using a fork of helm-docs containing the proposed fix and adjusting what needs to be adjusted to have a minimally readable and correct reference.

Goals:

Propose that we use the fork of helm-docs until Consider only the last group of comments starting with '# --'. norwoodj/helm-docs#99 is merged.
That all Helm chart fields present in the tables have a readable, correct and meaningful description.
- I can definitely use a second pair of eyes on this one 🙂
Include a comment on helm-values.rst indicating it is automatically generated.
For the same reason, add helm-values.rst to .gitattributes.

Non-goals:

That all Helm chart fields have an actual description.
To fix all inconsistency issues (e.g. some descriptions end with a . and some don't, some descriptions use backticks for correct rendering of code, some don't).
To make the tables extra-readable and actually eye-pleasant.

I think that these non-goals require a big effort and are only ever useful if the introduction of the fork is consensual, so I think we could cover them as part of a subsequent PR. Thoughts?

Improve the Helm chart documentation.

qmonnet

Looks great overall, thanks!

Please find a few comments and questions below.

Two additional remarks (that don't necessarily have to be addressed in the current PR):

I think that Documentation/helm-values.rst could be added to .gitattributes so that GitHub automatically folds its diff by default for reviews, given that it is a generated file?
Do you think we should have a commented header in Documentation/helm-values.rst that states that the file is generated, just like for Documentation/cmdref/cilium-agent.md for example? That might limit the risk of having contributors opening PRs to edit it directly?

install/kubernetes/cilium/values.yaml

qmonnet

The changes look good to me.

Please update the description of the PR to document your latest change, and please also document it in your commit log (I would recommend against empty commit logs in general).

Not blocking here, but my preference would even be to have several commits in this PR (one for the helm-docs image change, one for the doc updates, one for .gitattribute and “auto-generated” header, for example). I find it easier for understanding logical changes.

'helm-docs' has a bug which causes it to include comments belonging to previously-appearing but commented-out fields. A fix has been proposed in norwoodj/helm-docs#99, but hasn't been reviewed yet. While said PR doesn't get merged it's preferable to switch to a fork containing the fix so we can have a proper description for our Helm chart fields. Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

Adding so that GitHub automatically folds its diff by default for reviews given that it is a generated file. Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

Include a comment in 'helm-values.rst' indicating that the file is generated automatically. This will hopefully limit the risk of having contributors opening PRs to edit it directly. Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

Some fields appear in the 'Helm Reference' page without an associated description. This commit aims at fixing that. Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

qmonnet · 2021-06-08T12:34:04Z

install/kubernetes/cilium/values.yaml

@@ -1469,7 +1483,7 @@ preflight:
  # -- Annotations to be added to preflight pods
  podAnnotations: {}

-  # Labels to be added to preflight pods
+  # -- Labels to be added to thee preflight pod.


Thou should'st consider speaking 'i a moe recent English, haply.

Forsooth - thanketh thee f'r noticing t!

Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

qmonnet

All good, thanks a lot!

errordeveloper

Thanks for doing this @bmcustodio, LGTM!

kaworu

Thanks!

kaworu

Backporting this patch to v1.10 and v1.9 won't be trivial as the Helm doc stuff has moved since the releases. I think we should abandon attempting to backport to v1.8 since our Helm charts were very different and also I don't think we generated anything using Helm doc at the time.

@bmcustodio can you consider making PRs for v1.10 and v1.9 instead of the need-backport labels please? I think it would save some time to the backporting team.

Documentation/spelling_wordlist.txt

bmcustodio · 2021-06-15T15:54:18Z

@kaworu that works for me, I'll remove the tags 👍

pchaigno · 2021-06-25T16:40:36Z

@bmcustodio Shouldn't there be backport-done/1.10 and backport-done/1.9 labels here?

bmcustodio · 2021-06-28T09:14:18Z

@pchaigno I think so, yes 🤔 Maybe @errordeveloper can chime in?

pchaigno · 2021-06-28T09:18:08Z

I've added the labels.

bmcustodio requested review from a team as code owners June 8, 2021 08:35

bmcustodio requested a review from a team June 8, 2021 08:35

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 8, 2021

bmcustodio requested review from michi-covalent, qmonnet and kaworu June 8, 2021 08:35

maintainer-s-little-helper bot assigned michi-covalent Jun 8, 2021

bmcustodio requested a review from errordeveloper June 8, 2021 08:35

maintainer-s-little-helper bot assigned qmonnet, kaworu and errordeveloper Jun 8, 2021

qmonnet requested changes Jun 8, 2021

View reviewed changes

install/kubernetes/cilium/values.yaml Show resolved Hide resolved

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved

qmonnet changed the title ~~Improve the Helm chart documentation.~~ helm: Improve the Helm chart documentation. Jun 8, 2021

qmonnet added the release-note/misc This PR makes changes that have no direct user impact. label Jun 8, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 8, 2021

qmonnet added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Jun 8, 2021

bmcustodio requested a review from a team as a code owner June 8, 2021 10:03

qmonnet reviewed Jun 8, 2021

View reviewed changes

bmcustodio added 3 commits June 8, 2021 12:13

Add 'helm-values.rst' to '.gitattributes'.

500ed54

Adding so that GitHub automatically folds its diff by default for reviews given that it is a generated file. Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

bmcustodio requested a review from qmonnet June 8, 2021 11:23

Add missing descriptions to 'Helm Reference'.

7421973

Some fields appear in the 'Helm Reference' page without an associated description. This commit aims at fixing that. Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

qmonnet reviewed Jun 8, 2021

View reviewed changes

Fix typo.

8185b62

Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>

bmcustodio requested a review from qmonnet June 8, 2021 12:47

qmonnet approved these changes Jun 8, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned qmonnet Jun 8, 2021

bmcustodio added needs-backport/1.9 labels Jun 8, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.9.9 Jun 8, 2021

bmcustodio added the needs-backport/1.8 label Jun 8, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.8.11 Jun 8, 2021

qmonnet mentioned this pull request Jun 9, 2021

Documentation fixups #16482

Closed

errordeveloper approved these changes Jun 14, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned errordeveloper Jun 14, 2021

kaworu approved these changes Jun 15, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned kaworu Jun 15, 2021

kaworu reviewed Jun 15, 2021

View reviewed changes

Documentation/spelling_wordlist.txt Show resolved Hide resolved

bmcustodio removed needs-backport/1.8 labels Jun 15, 2021

maintainer-s-little-helper bot removed this from Needs backport from master in 1.9.9 Jun 15, 2021

maintainer-s-little-helper bot removed this from Needs backport from master in 1.8.11 Jun 15, 2021

bmcustodio removed the needs-backport/1.10 label Jun 17, 2021

errordeveloper added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 25, 2021

errordeveloper merged commit 776af64 into cilium:master Jun 25, 2021

This was referenced Jun 25, 2021

Better error reporting/catching in agent on nativeRoutingCIDR #16646

Merged

v1.10 — backport #16469 #16653

Merged

v1.9 — backport #16238 and #16469 #16645

Merged

bmcustodio deleted the bruno-helm-chart-documentation-improvements branch June 28, 2021 09:14

pchaigno added the backport-done/1.9 label Jun 28, 2021

maintainer-s-little-helper bot added this to Backport done to v1.9 in 1.9.9 Jun 28, 2021

pchaigno added the backport-done/1.10 label Jun 28, 2021

joestringer mentioned this pull request Jul 19, 2021

Prepare for release v1.9.9 #16932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm: Improve the Helm chart documentation. #16469

helm: Improve the Helm chart documentation. #16469

bmcustodio commented Jun 8, 2021 •

edited

qmonnet left a comment

qmonnet left a comment

qmonnet Jun 8, 2021

bmcustodio Jun 8, 2021

qmonnet left a comment

errordeveloper left a comment

kaworu left a comment

kaworu left a comment •

edited

bmcustodio commented Jun 15, 2021

pchaigno commented Jun 25, 2021

bmcustodio commented Jun 28, 2021

pchaigno commented Jun 28, 2021

helm: Improve the Helm chart documentation. #16469

helm: Improve the Helm chart documentation. #16469

Conversation

bmcustodio commented Jun 8, 2021 • edited

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet Jun 8, 2021

Choose a reason for hiding this comment

bmcustodio Jun 8, 2021

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

errordeveloper left a comment

Choose a reason for hiding this comment

kaworu left a comment

Choose a reason for hiding this comment

kaworu left a comment • edited

Choose a reason for hiding this comment

bmcustodio commented Jun 15, 2021

pchaigno commented Jun 25, 2021

bmcustodio commented Jun 28, 2021

pchaigno commented Jun 28, 2021

bmcustodio commented Jun 8, 2021 •

edited

kaworu left a comment •

edited