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.

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

Auto generate README.md, Chart.yaml, values.schema.json and values.yaml using tem & helm-jsonschema-gen #4209

Closed
wants to merge 1 commit into from

Conversation

inteon
Copy link
Member

@inteon inteon commented Jul 13, 2021

What this PR does / why we need it:
Alternative implementation for solving #4199.
It uses a tool called tem, that I created.
tem combines the functionality of kubepack.dev/chart-doc-gen and github.com/gomatic/renderizer/v2.
It also allows us to create custom table layouts.
tem does not ONLY work for Helm, it can be used for any situation where templating using YAML as input is useful.

UPDATE: as requested by @wallrj, now the values.schema.json file is also auto-generated.
This is done using another custom tool https://github.com/amurant/helm-jsonschema-gen, that uses go code and
translates it to a JSON schema definition.

Which issue this PR fixes

fixes #4010
fixes #2891

Release note:

The Helm chart README.md, Chart.yaml, values.yaml, values.schema.json files are now generated automatically

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/deploy Indicates a PR modifies deployment configuration needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 13, 2021
@jetstack-bot
Copy link
Contributor

Hi @inteon. Thanks for your PR.

I'm waiting for a jetstack or cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@jetstack-bot jetstack-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 13, 2021
@irbekrm
Copy link
Contributor

irbekrm commented Jul 13, 2021

/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 13, 2021
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @inteon

tem seems really cool. Useful to be able to apply Helm style templating generally, plus the added parsing of comments.
I haven't reviewed the code though.

I've got some general questions about this approach:

  1. If we agree that we can improve the installation experience by creating a Helm JSON schema, which will help prevent users from setting invalid values. Then the json schema file seems like the natural place to add value descriptions (rather than in comments in the Yaml). And json schema supports examples and defaults annotations too which would make those things much easier to extract and avoid parsing the comment strings. https://json-schema.org/understanding-json-schema/reference/generic.html#annotations
  2. And I had a vague idea that the JSON schema file might serve as the basis for generating command line flags that reflect the available values, e.g. kubectl cert-manager install --cainjector.enabled=false --controller.resources.requests.memory=200Mi.
  3. Or it could be used to generate an overview of the available values in kubectl cert-manager install --help.
  4. The generated README file in this branch creates a table which scrolls horizontally, way beyond the page margin, which makes it very difficult to read: https://github.com/jetstack/cert-manager/blob/2026972af9a74d62dd0adcce99b9c90e0cf43534/deploy/charts/cert-manager/README.md#configurationThe original table has wrapped cell content: https://github.com/jetstack/cert-manager/blob/master/deploy/charts/cert-manager/README.template.md#configuration
  5. I dislike the complicated template logic in README.template.md which is used to create the table of values. I find it difficult to interpret. Isn't there a markdown generator which can take a slice of structs and turn it into a table?
  6. If tem is used, can it not be downloaded as a binary rather than adding its Go dependencies to those of cert-manager, which are already too sprawling.

Please pick holes these arguments if you disagree.

@inteon inteon force-pushed the helm_autogen_tem branch 2 times, most recently from 408b988 to e9ed842 Compare July 15, 2021 13:19
@inteon
Copy link
Member Author

inteon commented Jul 15, 2021

  1. & 2) I agree that ideally cert-manager should have a json-schema added to its chart. I fear however, that it will be very hard to manage such a schema directly. Charts that use this schema often only use it to partially describe the values.yaml file.
    eg. https://github.com/bitnami/charts/blob/master/bitnami/redis/values.yaml vs https://github.com/bitnami/charts/blob/master/bitnami/redis/values.schema.json

  2. kubectl cert-manager install --help -> Yes, we should add more info to the install command. This can be done by using json-schema OR just the values.yaml file. json-schema could add some validation, but might be harder to maintain (see 1&2)

  3. That version contained an error that removed the newlines. Somehow, github accepts <br> as a newline only if the "lang" attribute of the surrounding <pre> tag is not set.
    The latest version should be rendered better:
    https://github.com/jetstack/cert-manager/blob/e9ed8426b8b6d92a6217ed0efc6dcdc6799620a7/deploy/charts/cert-manager/README.md

  4. This is the best way I have found to make the table-generation more modular. This allows cert-manager to specify custom logic for rendering the comments above a values.yaml property. Most of this logic is related to rendering the yaml examples and using the correct characters in the table. https://github.com/kubepack/chart-doc-gen hardcodes this, but that means that in some situations it might not work eg. Show multi-value lines inside <code> block #31 kubepack/chart-doc-gen#32.

  5. fixed

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks for downloading the binaries and making the darwin binaries available.

I'm still in favour of using the jsonschema as the source of all this information...I think it could even be used to generate the values.yaml file.

ArtifactHub does a pretty nice job of presenting the schema: https://artifacthub.io/packages/helm/bitnami/redis?modal=values-schema

Which makes me think we could even drop the table of values from the README.

Let's discuss it tomorrow.

deploy/charts/cert-manager/README.template.md Outdated Show resolved Hide resolved
hack/bin/deps.bzl Outdated Show resolved Hide resolved
deploy/charts/cert-manager/values.yaml Outdated Show resolved Hide resolved
@wallrj
Copy link
Member

wallrj commented Jul 15, 2021

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2021
@inteon inteon force-pushed the helm_autogen_tem branch 4 times, most recently from 67c55bf to 964a255 Compare July 29, 2021 16:00
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2021
@inteon inteon changed the title Auto generate README.md & template Chart.yaml using tem Auto generate README.md, Chart.yaml, values.schema.json and values.yaml using tem & helm-jsonschema-gen Jul 29, 2021
hack/update-helmdocsgen.sh Outdated Show resolved Hide resolved
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2021
@inteon
Copy link
Member Author

inteon commented Oct 26, 2021

blocked on #4556

@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2021
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2021
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2021
@inteon
Copy link
Member Author

inteon commented Jan 6, 2022

blocked on improvements to the dependencies. (I'm working on helm-jsonschema-gen and tem improvements)

@jetstack-bot
Copy link
Contributor

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

@jetstack-bot jetstack-bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 6, 2022
@jetstack-bot
Copy link
Contributor

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

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2022
@inteon
Copy link
Member Author

inteon commented Jul 6, 2022

/remove-lifecycle stale

@jetstack-bot jetstack-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2022
@inteon inteon marked this pull request as draft August 30, 2022 08:34
@jetstack-bot jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2022
Signed-off-by: Inteon <42113979+inteon@users.noreply.github.com>
@jetstack-bot jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 12, 2022
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon, wallrj

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot
Copy link
Contributor

@inteon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cert-manager-bazel 3100f34 link /test pull-cert-manager-bazel
pull-cert-manager-master-chart 3100f34 link true /test pull-cert-manager-master-chart
pull-cert-manager-master-make-test 3100f34 link true /test pull-cert-manager-master-make-test
pull-cert-manager-master-e2e-v1-24 3100f34 link true /test pull-cert-manager-master-e2e-v1-24
pull-cert-manager-master-e2e-v1-25 3100f34 link true /test pull-cert-manager-master-e2e-v1-25
pull-cert-manager-master-e2e-v1-24-upgrade 3100f34 link true /test pull-cert-manager-master-e2e-v1-24-upgrade
pull-cert-manager-master-e2e-v1-25-upgrade 3100f34 link true /test pull-cert-manager-master-e2e-v1-25-upgrade
pull-cert-manager-master-e2e-v1-26-upgrade d00da3c link true /test pull-cert-manager-master-e2e-v1-26-upgrade

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@SgtCoDFish SgtCoDFish closed this Feb 14, 2023
@inteon
Copy link
Member Author

inteon commented Feb 14, 2023

will pick this up later in another PR

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/deploy Indicates a PR modifies deployment configuration dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
5 participants