Use better flags-docs creator #7811
Conversation
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
added
kind/documentation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omerap12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XL
Signed-off-by: Omer Aplatony <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
There was a problem hiding this comment.
Thanks for taking another shot at this! I think the parsing doesn't work reliably on all occasions, though:
- Some occurrences of
type: If - Some descriptions repeat the parameter name, like with
--add-header-dir
If the additional type column makes our lives harder (because not all parameters seem to have it, not sure why?), then I guess it is also ok to just drop it.
Signed-off-by: Omer Aplatony <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
I saw that CA also doesn't use type so I removed it |
|
This link needs to be updated, if the filename changes: |
Yeah I forgot 😄 |
Signed-off-by: Omer Aplatony <[email protected]>
omerap12
force-pushed
the
fix-vpa-flags-docs
branch
from
736bf85 to
9fba59b
Compare
|
This may be a bit of a nit, but in the rendered view some of the long flags are wrapped: May be if they were code snippets it would help not wrapping them? |
Signed-off-by: Omer Aplatony <[email protected]>
done |
|
Damn, that didn't fix it: |
| | `--tls-cert-file` | "/etc/tls-certs/serverCert.pem" | Path to server certificate PEM file. | | ||
| | `--tls-ciphers` | | A comma-separated or colon-separated list of ciphers to accept. Only works when min-tls-version is set to tls1_2. | | ||
| | `--tls-private-key` | "/etc/tls-certs/serverKey.pem" | Path to server certificate key PEM file. | | ||
| | `--v,` | 4 | Level set the log level verbosity | |
There was a problem hiding this comment.
Damn, not sure how it's got there
There was a problem hiding this comment.
I came up with a workaround for this (I know it's ugly). Let me know what you think
| echo "|------|---------|-------------|" | ||
|
|
||
| $binary --help 2>&1 | grep -E '^\s*-' | while read -r line; do | ||
| flags=$(echo "$line" | awk '{print $1, $2}' | grep -oP '(?<=-)([^ ]+(?:, --[^ ]+)?)') |
There was a problem hiding this comment.
The -P flag doesn't work for macOS, which I assume developers running this may use, so I guess that needs to be replaced with something else
There was a problem hiding this comment.
Hmm, I'm using Linux (you know why lol), so I'll try to find a suitable replacement for this flag that works on both OSs.
Signed-off-by: Omer Aplatony <[email protected]>
omerap12
force-pushed
the
fix-vpa-flags-docs
branch
from
f646777 to
66cf05c
Compare
|
|
||
| echo "# What are the parameters to VPA ${component}?" | ||
| echo "This document is auto-generated from the flag definitions in the VPA ${component} code." | ||
| echo "Last updated: $(date -u '+%Y-%m-%d %H:%M:%S UTC')" |
There was a problem hiding this comment.
While I appreciate having the option to see right away how up-to-date this document is, I think we shouldn't include a mechanism which creates a diff in every run. So let's remove the timestamp and instead rely on the change history for this file?
This way, we could use running ./hack/generate-flags.sh as a way to see if the flags are up-to-date or if there is a diff. We could also include this e.g. in a PR check, asking people to run this and keep the docs in sync with the code.
There was a problem hiding this comment.
Agree, I'll adjust. Once this is merged, I'll open an issue to address it.
|
With this PR, do we still need |
Yes, I think we can remove it |
Co-authored-by: Marco Voelz <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
|
Any idea if adding a table of contents will be possible? |
Hmm, do you really think that's a good idea? In my opinion, the document is quite small, but I can add something if you think it would be an improvement. |
|
btw, unit tests are failing.. @voelzmo @adrianmoisey |
Yeah, I'm not strongly opinionated on it. It might be nice as the doc only contains 3 sections, and each one requires a bit of scrolling. |
I wonder if we have a flake, can you re-run then? (I can't click retry) |
Signed-off-by: Omer Aplatony <[email protected]>
|
Passed now lol @adrianmoisey |
| fi | ||
|
|
||
| echo "# What are the parameters to VPA ${component}?" | ||
| echo "This document is auto-generated from the flag definitions in the VPA ${component} code." |
There was a problem hiding this comment.
I wonder if it makes sense to mention that it's autogenerated from the default branch?
I could see someone reading the docs in master, assuming that a newly added flag will work for them in their release.
This makes me think that we should also consider how we're showing versioned docs when we release new versions of the VPA. But that's a discussion for a different thread.
There was a problem hiding this comment.
Yeah, I can add this to the script. But a follow-up question - how can a user check if some feature is available in their current VPA version?
There was a problem hiding this comment.
Yeah, I can add this to the script. But a follow-up question - how can a user check if some feature is available in their current VPA version?
Yeah, that's tough.
At the moment they can (in theory) browse the repo at the release branch for their current version. But that's something they need to figure out themselves.
Signed-off-by: Omer Aplatony <[email protected]>
|
/lgtm Overall I think this is a good improvement. The markdown table is a little strange, but there's not much we can do about that. |
k8s-ci-robot
added
the
lgtm
Thanks, I guess we can have more people tweaking it to improve it (since I'm not great at this kind of thing). |
What type of PR is this?
/kind documentation
/kind cleanup
What this PR does / why we need it:
Better generation of flags docs.
Which issue(s) this PR fixes:
Fixes #7779
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: