Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Migrate to readme-generator for helm maintained by bitnami #431

Merged

Conversation

krishnakv
Copy link
Contributor

Updated the default values.yaml with the annotations for readme-generator-for-helm. Only the values in the existing values.yaml file have been retained.

Also updated the README.md file. Unlike helm-docs, readme-generator just expects a parameters section and appends the list of values to the README.md file. There is no separate README template file required.

I have marked this "Draft" as there are a few more changes required if we proceed with this templating tool. For example README.md.gotmpl needs to be removed and pipeline scripts need to be modified.

Resolves issue #407

@marcofranssen
Copy link
Contributor

Could you update the helm-docs.sh script as well to use this other tool, so we have the complete picture. I want to experiment with it a bit as well to get a better understanding on this tool

Krishnakumar Venkataraman added 2 commits August 18, 2023 12:24
Signed-off-by: Krishnakumar Venkataraman <[email protected]>
Signed-off-by: Krishnakumar Venkataraman <[email protected]>
@krishnakv
Copy link
Contributor Author

Hi @marcofranssen , looks like this PR has got closed as there were conflicting commits I pulled into my fork. I am working through installing readme-generator cleanly into helm-docs.sh - its a node app. Will open a new PR with helm-docs.sh updated before our next call. Just FYI.

@krishnakv
Copy link
Contributor Author

Reopening the pull request, will fix the merge conflicts and track changes under this PR itself.

@krishnakv krishnakv reopened this Aug 18, 2023
@krishnakv
Copy link
Contributor Author

Hi @marcofranssen , folks, have added readme-gen.sh for the readme-generator install. I tried various ways to install it as a binary, however, this tool is coded in node, so could not successfully get a binary build. In the end, went with the default install recommendations and tested.

Copy link
Contributor

@marcofranssen marcofranssen 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 cleaning up the PR and including the script.

Few suggestions on the PR so far. In addition I was trying to find how this tool is more customizable as the other one we are using. So far I haven't found any documentation on how we can influence the way the documentation is generated. Do you have a link to tool documentation on how to use it? E.g. like this tool has https://github.com/norwoodj/helm-docs#markdown-rendering

  • Are there any variables we can use in templates?
  • Can we define a template?
  • What are the major cons of this tool compared to helm-docs?

What I see currently is that all is grouped by dependency or global? Is that out of the box functionality? can we influence the grouping?

Is everything outside of the Parameters part of the static template?

readme-gen.sh Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

can we replace the original file helm-docs.sh instead of adding a new file? This way we don't have to update contributing.md as well the pipeline remains to work and safegaurds us for outdated docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The readme file is available at the bitnami readme-generator-for-helm repo. There are a limited number of tags "@param", "@Skip", "@section", "@extra" etc.

There is also only support for generating the values table under the "## Parameters" section in README.md. Other templating options such as introducing maintainers, chart version and other details into the README is not supported.

There is the option to divide the docs into different sections using "@section" tags. I have also retained the filename for helm-docs.sh.

All the values files including sub-charts are updated, so this PR can be taken for a test drive on your local setup. :-)

readme-gen.sh Outdated
# platform agnostic npm install, also adds into the path
npm install -g "@bitnami/readme-generator-for-helm@${README_GENERATOR_VERSION}"

if [[ -z '$(type -d "${README_GENERATOR_EXE}")' ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

This checks against the directory, but not the executable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, fixed that.

readme-gen.sh Outdated Show resolved Hide resolved
readme-gen.sh Outdated Show resolved Hide resolved
@marcofranssen
Copy link
Contributor

Hi @marcofranssen , folks, have added readme-gen.sh for the readme-generator install. I tried various ways to install it as a binary, however, this tool is coded in node, so could not successfully get a binary build. In the end, went with the default install recommendations and tested.

Yup already expected that, so that just means in out workflow we need to add the setup-node action so we have npm available.

@kfox1111
Copy link
Contributor

What about running it from a container so we don't have to manage node on the host?

@faisal-memon faisal-memon added this to the 0.13.0 milestone Aug 21, 2023
Signed-off-by: Krishnakumar Venkataraman <[email protected]>
@faisal-memon
Copy link
Contributor

Thanks @krishnakv for taking this on. The sections make it so much easier to read. If this is ready for review, do you mind taking if off draft mode?

@krishnakv krishnakv marked this pull request as ready for review September 2, 2023 09:30
@krishnakv
Copy link
Contributor Author

Thanks @krishnakv for taking this on. The sections make it so much easier to read. If this is ready for review, do you mind taking if off draft mode?

Done, @faisal-memon .

Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

Some inline suggestions and improvements.

Also tried it locally but got some error

$ ./helm-docs.sh
readme-generator --values=./charts/spire/values.yaml --readme=./charts/spire/README.md
INFO: Checking missing metadata...
INFO: Metadata is correct!
INFO: Found parameters section at line: 107
INFO: Found section end at line: 182
INFO: Last parameter table line found at line: 180
INFO: Inserting the new table into the README...
readme-generator --values=./charts/spire/charts/tornjak-frontend/values.yaml --readme=./charts/spire/charts/tornjak-frontend/README.md
Skipping check for global
Skipping check for imagePullSecrets
Skipping check for serviceAccount.annotations
Skipping check for labels
Skipping check for podSecurityContext
Skipping check for securityContext
Skipping check for nodeSelector.kubernetes.io/arch
Skipping check for affinity
Skipping check for tolerations
Skipping check for topologySpreadConstraints
INFO: Checking missing metadata...
INFO: Metadata is correct!
INFO: Found parameters section at line: 55
INFO: The parameters section seems to be the last section in the file
INFO: Inserting the new table into the README...
readme-generator --values=./charts/spire/charts/spiffe-oidc-discovery-provider/values.yaml --readme=./charts/spire/charts/spiffe-oidc-discovery-provider/README.md
Skipping check for global
Skipping check for annotations
Skipping check for resources
Skipping check for configMap.annotations
Skipping check for podSecurityContext
Skipping check for securityContext
Skipping check for podAnnotations
Skipping check for config.additionalDomains
Skipping check for imagePullSecrets
Skipping check for nodeSelector
Skipping check for tolerations
Skipping check for affinity
Skipping check for telemetry.prometheus.podMonitor.labels
Skipping check for telemetry.prometheus.nginxExporter.resources
Skipping check for ingress.annotations
Skipping check for ingress.hosts[0].host
Skipping check for ingress.hosts[0].paths[0].path
Skipping check for ingress.hosts[0].paths[0].pathType
Skipping check for ingress.tls
INFO: Checking missing metadata...


######
The following errors must be fixed before proceeding
######


ERROR: Missing metadata for key: tests.hostAliases
ERROR: Missing metadata for key: tests.tls.enabled
ERROR: Missing metadata for key: tests.tls.customCA
/usr/local/lib/node_modules/@bitnami/readme-generator-for-helm/lib/checker.js:93
    throw new Error('ERROR: Wrong metadata!');
    ^

Error: ERROR: Wrong metadata!
    at checkKeys (/usr/local/lib/node_modules/@bitnami/readme-generator-for-helm/lib/checker.js:93:11)
    at getParsedMetadata (/usr/local/lib/node_modules/@bitnami/readme-generator-for-helm/index.js:26:3)
    at runReadmeGenerator (/usr/local/lib/node_modules/@bitnami/readme-generator-for-helm/index.js:52:28)
    at Object.<anonymous> (/usr/local/lib/node_modules/@bitnami/readme-generator-for-helm/bin/index.js:22:1)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47

Node.js v18.17.1

charts/spire/charts/spire-server/README.md Outdated Show resolved Hide resolved
release-chart.sh Outdated Show resolved Hide resolved
helm-docs.sh Outdated Show resolved Hide resolved
helm-docs.sh Outdated Show resolved Hide resolved
helm-docs.sh Outdated Show resolved Hide resolved
helm-docs.sh Outdated Show resolved Hide resolved
Krishnakumar Venkataraman and others added 2 commits September 5, 2023 17:56
README.md Outdated Show resolved Hide resolved
@faisal-memon faisal-memon added the documentation Improvements or additions to documentation label Sep 6, 2023
Krishnakumar Venkataraman added 2 commits September 7, 2023 15:02
Signed-off-by: Krishnakumar Venkataraman <[email protected]>
@krishnakv
Copy link
Contributor Author

@marcofranssen , @kfox1111 , @faisal-memon , all your comments must be addressed at this time. Please do check and raise if any other issues.

@marcofranssen
Copy link
Contributor

Thanks, I pushed the fix for CI (docs where outdated), but forgot to signoff my commit. As the branch is on your Fork I can't force push to fix it up. Could you add the signoff to fix it up?

git pull --rebase
git commit --amend
git push --force-with-lease

😊

Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

LGTM.

Just needs the DCO issue fixed.

Copy link
Contributor

@faisal-memon faisal-memon left a comment

Choose a reason for hiding this comment

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

Thanks @krishnakv . This was a huge effort.

@marcofranssen marcofranssen merged commit 65d5695 into spiffe:main Sep 8, 2023
43 of 44 checks passed
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <[email protected]>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <[email protected]>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <[email protected]>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants