-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
docs(charts): using helm-docs for chart #8061
Conversation
@sc250024: This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Welcome @sc250024! |
Hi @sc250024. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@cpanato @ElvinEfendi Any comments on this? |
@sc250024 this is amazing!! Thanks for the PR. I'm not a helm expert, but will follow on this one and also ping the dev team so someone can take a look into the implications. But the idea is great! :) |
/assign |
LGTM! helm-docs is a good tool. I will review the template and related configuration later. /assign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks cool!
/approve
@rikatz Looks like we just need your input. Would you mind approving please? Or do you have any more suggestions? |
I just want to take a final look before approving :) will do between today and tomorrow. Thanks! |
/assign |
Cool, thank you @rikatz ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, left some comments.
/ok-to-test Overall this is great, and by the same time it's so ugly that we have a full Readme and no comments in the fields / values :D Seems like a good first issue to fix that in the chart, so we may have everything properly documented, + any new value on it should be documented as well. Thanks @sc250024 ! |
This enables the use of the `helm-docs` tool on the Helm chart located in `charts/ingress-nginx`. This will make it possible to automatically document new variables in the `values.yaml` file. Signed-off-by: Scott Crooks <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, rikatz, sc250024, tao12345666333 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@rikatz Is this good for a merge finally? Thank you! |
@rikatz Ping 🤙 |
/hold cancel |
@rikatz It looks like the job has failed => https://github.com/kubernetes/ingress-nginx/runs/4747631693?check_suite_focus=true . Do you know whom to notify about this error?
@aledbf Maybe? |
Nope it's with me. I will try to look this week, ok? |
This enables the use of the `helm-docs` tool on the Helm chart located in `charts/ingress-nginx`. This will make it possible to automatically document new variables in the `values.yaml` file. Signed-off-by: Scott Crooks <[email protected]>
Description
This enables the use of the helm-docs tool on the Helm chart located in
charts/ingress-nginx
. This will make it possible to automatically document new variables in thevalues.yaml
file.What this PR does / why we need it:
This PR improves the documentation for the chart so that possible configuration values are documented automatically.
It also adds CI checks so that moving forward, someone adding / removing / updating values in the Helm chart will have to make sure that
helm-docs
has been run.Types of changes
Which issue/s this PR fixes
Not applicable
How Has This Been Tested?
Testing is not necessary since the documentation has already been generated.
Checklist: