( 📖 ) [WiP] Enhancements to CLI Help#5498
( 📖 ) [WiP] Enhancements to CLI Help#5498cmallikarjunah wants to merge 1 commit intokubernetes-sigs:masterfrom
Conversation
|
Hi @cmallikarjunah. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. DetailsInstructions 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cmallikarjunah The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| fs.StringVar(&p.domain, "domain", "my.domain", "[Required] Domain for API groups (e.g., example.org). "+ | ||
| "This becomes the suffix for your API groups (e.g., crew.example.org/v1). "+ | ||
| "Should be a domain you control. "+ | ||
| "Cannot be changed after project initialization") |
There was a problem hiding this comment.
Thanks for taking a look! Where do you think we should add all this information for the flag?
Also, shouldn’t the flag text be concise?
There was a problem hiding this comment.
@camilamacedo86 @cmallikarjunah I was testing it out and it seems that the short descriptions are too long, so the text ends up being truncated:
$ kubebuilder init --
--domain ([Required] Domain for API groups (e.g., example.org). This becomes the suffix for your API groups (e.g., crew.example.org/v1). Should be a domain you control. Cannot be changed after project…)
--fetch-deps (download dependencies after scaffolding)
--help (help for init)
--license ([Configuration] license to use (apache2 or none, default: apache2))
--multigroup ([Layout] Enable multigroup layout to organize APIs by group. Scaffolds APIs in api/<group>/<version>/ instead of api/<version>/ Useful when managing multiple API groups (e.g., batch, apps, c…)
--namespaced ([Layout] Enable namespace-scoped deployment instead of cluster-scoped (default: cluster-scoped). Manager watches one or more specific namespaces instead of all namespaces. Namespaces to watc…)
--owner ([Configuration] copyright owner for license headers)
--plugins (plugin keys to be used for this subcommand execution)
--project-name (name of this project)
--project-version (project version)
--repo ([Configuration] Go module path (e.g., github.com/user/repo); auto-detected if not provided and project is initialized within $GOPATH)
--skip-go-version-check (skip Go version check)
I suggest we keep the line lenght under 120 characters and don't use brackets to avoid polluting the message visually.
There was a problem hiding this comment.
@camilamacedo86
I was trying to avoid duplication and to put info relating to each flag at one place. So I tried to accommodate all the flag description into its flag text. But I understand it may not look proper.
@vitorfloriano Thanks for verifying.
Will rework on this
| Webhooks remain cluster-scoped even in namespace-scoped mode. | ||
| The manager cache is restricted to WATCH_NAMESPACE, but webhooks receive requests | ||
| from ALL namespaces. You must configure namespaceSelector or objectSelector to align | ||
| webhook scope with the cache. |
There was a problem hiding this comment.
I think we should keep the description here right?
Why we think that not?
pkg/plugins/golang/v4/edit.go
Outdated
| "More info: https://book.kubebuilder.io/migration/namespace-scoped.html . ") | ||
| fs.BoolVar(&p.force, "force", false, "Overwrite existing scaffolded files to apply configuration changes. "+ | ||
| "Example: With --namespaced, regenerates config/manager/manager.yaml to add WATCH_NAMESPACE env var. "+ | ||
| "Warning: This overwrites default scaffold files; manual changes in those files may be lost.") |
There was a problem hiding this comment.
Shouldn’t the flag text be concise?
I am not sure, if this changes will make the UX better and ensure that we are following Cobra CLI best / common practices.
pkg/plugins/golang/v4/init.go
Outdated
| fs.StringVar(&p.license, "license", "apache2", | ||
| "license header to use (apache2 or none)") | ||
| fs.StringVar(&p.owner, "owner", "", "copyright owner for license headers") | ||
| "[Configuration] license to use (apache2 or none, default: apache2)") |
There was a problem hiding this comment.
Just a small recommendation,
Maybe it would be better if we follow some format regarding the supported values and also default value it sets if not provided across all.
I was wondering maybe something like this
(apache2 | none) (default "apache2")
There was a problem hiding this comment.
That should look good. Thanks for the suggestion @mayuka-c
pkg/plugins/golang/v4/init.go
Outdated
| fs.BoolVar(&p.multigroup, "multigroup", false, | ||
| "enable multigroup layout (organize APIs by group)") | ||
| "[Layout] Enable multigroup layout to organize APIs by group. "+ | ||
| "Scaffolds APIs in api/<group>/<version>/ instead of api/<version>/ "+ | ||
| "Useful when managing multiple API groups (e.g., batch, apps, crew). ") |
There was a problem hiding this comment.
I guess the description of the flag should be limited to around 80 chars. This long description if required we can keep it in command help?
5199364 to
1941ec0
Compare
Layout changes for better UX: Moved flag descriptions to Flags section i.e; to BindFlags() in the code. Some advantages of having detailed descriptions in Flags section:
Issue: Enhancement: Review and improve flag descriptions, examples, and CLI help for better UX #5446