close
The Wayback Machine - https://web.archive.org/web/20210916235720/https://github.com/jetstack/cert-manager/issues/2891
Skip to content

/ cert-manager Public

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

Jump to bottom

[feature - helm chart] Auto document the Parameters table using helm-docs #2891

Open
sudermanjr opened this issue May 5, 2020 · 8 comments · May be fixed by #4209
Open

[feature - helm chart] Auto document the Parameters table using helm-docs #2891

sudermanjr opened this issue May 5, 2020 · 8 comments · May be fixed by #4209
Assignees
@edeediong
Labels
area/deploy good first issue help wanted kind/feature lifecycle/stale priority/backlog

Comments

@sudermanjr
Copy link
Contributor

@sudermanjr sudermanjr commented May 5, 2020

Describe the solution you'd like
helm-docs Is a nice little utility for auto-generating the Parameters table from comments in the values.yaml file. It would make PRs into the chart much easier, because you don't need to document in multiple places, and you don't need to edit a huge markdown table.

If you're open to this, I could probably work a PR for it.

/kind feature
@munnerz
Copy link
Collaborator

@munnerz munnerz commented May 11, 2020

This would be great 😄 it'd need to be incorporated into our current Bazel build pipeline, but we already template the README file so it should be possible to use a similar technique and inject this tool into the build pipeline for the README.md file.

It should be possible to add an additional Bazel build rule chained together and similar to

cert-manager/deploy/charts/cert-manager/BUILD.bazel

Lines 29 to 39 in ba7871d

genrule(
name = "README",
srcs = [
"README.template.md",
"//:version",
],
outs = ["README.md"],
cmd = "cat $(location README.template.md) | sed -e \"s:{{RELEASE_VERSION}}:$$(cat $(location //:version)):g\" > $@",
stamp = 1,
visibility = ["//visibility:public"],
)
which runs this tool and templates out the doc's contents.

@tuananh
Copy link

@tuananh tuananh commented Feb 1, 2021 •
edited

@munnerz if we were to use helm-docs, the commented values will not be generated in the values table. I'm not sure if that's what you wanted? if that's ok, i will create a PR

eg: leaseDuration field

cert-manager/deploy/charts/cert-manager/values.yaml

Lines 23 to 41 in 33f1881

leaderElection:
# Override the namespace used to store the ConfigMap for leader election
namespace: "kube-system"
# The duration that non-leader candidates will wait after observing a
# leadership renewal until attempting to acquire leadership of a led but
# unrenewed leader slot. This is effectively the maximum duration that a
# leader can be stopped before it is replaced by another candidate.
# leaseDuration: 60s
# The interval between attempts by the acting master to renew a leadership
# slot before it stops leading. This must be less than or equal to the
# lease duration.
# renewDeadline: 40s
# The duration the clients should wait between attempting acquisition and
# renewal of a leadership.
# retryPeriod: 15s

@edeediong
Copy link

@edeediong edeediong commented Mar 23, 2021 •
edited

@tuananh You're right that running the helm-docs directly on the helm manifests doesn't display the description from values too. However, having looked at the documentation, we can attach -- to each comment and the tool will generate description for the active values.

However, I do not know the workaround of leaseDuration field being skipped.

@edeediong
Copy link

@edeediong edeediong commented Mar 23, 2021

/assign @edeediong

@edeediong
Copy link

@edeediong edeediong commented Mar 23, 2021

@jakexks @wallrj I figured out how to work around the issue. As I mentioned above, I'll need to edit all comments appending -- to each description so that helm-docs can pick it up. As for the bazel build, I'm still trying to figure how to add another chain into the Pipeline.

@wallrj
Copy link
Member

@wallrj wallrj commented Mar 24, 2021

@jakexks @wallrj I figured out how to work around the issue. As I mentioned above, I'll need to edit all comments appending -- to each description so that helm-docs can pick it up. As for the bazel build, I'm still trying to figure how to add another chain into the Pipeline.

Sounds promising @edeediong

Maybe you could start with a ./hack/update-helm-doc.sh script which would directly update the README.template.md file

And an accompanying ./hack/verify-helm-doc.sh script which exits with a >0 exit code and a diff if, after running update-helm-docs.sh, there are found to be any changes.

That's how the ./verify-gofmt.sh and ./verify-boilerplate.sh behave, so it'd be consistent.

I prefer this to auto generating and injecting the table during bazel builds, because it's going to be easier to see rendered table during code reviews of PRs that change the helm chart values.

My 2c.

@wallrj
Copy link
Member

@wallrj wallrj commented Mar 24, 2021

@tuananh You're right that running the helm-docs directly on the helm manifests doesn't display the description from values too. However, having looked at the documentation, we can attach -- to each comment and the tool will generate description for the active values.

That sounds good. I see some discussion of this syntax in norwoodj/helm-docs#67

However, I do not know the workaround of leaseDuration field being skipped.

I read norwoodj/helm-docs#84 which suggests to me that if we uncomment leaseDuration field in the values.yaml file, but without a value, that helm will not change that value in the templates....but that needs testing.
And even still, that bug report suggests that there's a bug in helm-doc and that the generated table won't include the documentation for these nil value fields.
Worth investigating though.

@inteon inteon mentioned this issue Jul 9, 2021
Closed
@inteon inteon linked a pull request that will close this issue Jul 13, 2021
Open
@jetstack-bot
Copy link
Collaborator

@jetstack-bot jetstack-bot commented Sep 16, 2021

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@edeediong edeediong

Labels
area/deploy good first issue help wanted kind/feature lifecycle/stale priority/backlog
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Auto generate README.md, Chart.yaml, values.schema.json and values.yaml using tem & helm-jsonschema-gen
Auto generate README.md & template Chart.yaml
6 participants
@munnerz @tuananh @wallrj @sudermanjr @jetstack-bot @edeediong