fix CLI reference generation for current and next versions#811
fix CLI reference generation for current and next versions#811jetstack-bot merged 2 commits intocert-manager:masterfrom jahrlin:master
Conversation
generate current docs at the correct place generate next docs with CLI Signed-off-by: Joakim Ahrlin <joakim.ahrlin@gmail.com>
jetstack-bot
added
dco-signoff: yes
|
✔️ Deploy Preview for cert-manager-website ready! 🔨 Explore the source changes: 1c28402 🔍 Inspect the deploy log: https://app.netlify.com/sites/cert-manager-website/deploys/61f16a99c42e860008d34b4e 😎 Browse the preview: https://deploy-preview-811--cert-manager-website.netlify.app |
wallrj
left a comment
wallrj
left a comment
There was a problem hiding this comment.
Thanks @jahrlin
I ran it on my laptop without errors.
And I see that the preview now shows the current webhook CLI flags rather than the flags in master: https://deploy-preview-811--cert-manager.netlify.app/docs/cli/webhook/
There are just one or two things I didn't understand about the script.
| # API versions are no longer served, but the apis are still in the codebase. We | ||
| # don't want to show the docs for those API versions so this is a workaround. | ||
| # This will only be necessary for the release-1.6 branch. | ||
| checkout "release-1.6" |
There was a problem hiding this comment.
This checkout and the comment above relate to the rm and gendocs command below,
so I think it the genclireference commands should be moved after those.
There was a problem hiding this comment.
Good point, I have moved the comment.
genclireference must happen before we delete the legacy API versions because the code does not compile without them. I added a comment on that as well
| genclireference "docs" "cmd/cainjector" "cainjector" | ||
| genclireference "docs" "cmd/ctl" "cmctl" | ||
| genclireference "docs" "cmd/controller" "controller" | ||
| genclireference "docs" "cmd/webhook" "webhook" |
There was a problem hiding this comment.
And if these lines are deliberately placed after the checkout command, then I'd add a comment explaining why it's important, so that someone doesn't come alone and rearrange this code.
There was a problem hiding this comment.
I have added a comment here covering this!
scripts/gendocs/generate
Outdated
| # and add them to the git index | ||
| # when adding a new version here use | ||
| # genversionwithcli to force generation of CLI docs | ||
| # and add them to the git index, then switch back to genversion |
There was a problem hiding this comment.
I don't understand this comment.
Does it mean that I should add another genversionwithcli line somewhere in the scipt.
Please add a little more detail about how this script should be modified in future.
There was a problem hiding this comment.
I have re-formulated the comment and included an example for the next change in this section
Signed-off-by: Joakim Ahrlin <joakim.ahrlin@gmail.com>
|
Thanks @wallrj - very good feedback and I have tried to simplify and clarify the comments |
|
/approve |
wallrj
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: jahrlin, wallrj The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
This PR fixes an issue where CLI reference docs for the current release was generated from master, meaning they would contain flags included in the upcoming release.
Signed-off-by: Joakim Ahrlin joakim.ahrlin@gmail.com