-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[AKS] Knack - improve help messages #5078
[AKS] Knack - improve help messages #5078
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/5078 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove the doc strings from methods in custom.py.
kubectl port-forward | ||
:type disable_browser: bool | ||
""" | ||
"""Open a web browser to the dashboard for a managed Kubernetes cluster.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this since it's already defined in help.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this string is redundant (the one in _help.py wins), but it felt wrong to leave the code with no docstring at all. I can remove them if you'd rather have that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay that's fine. Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually do prefer you remove them. The reason is we don't want to give anyone the impression that we encourage pulling help from custom command docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. I'll remove them.
until the managed cluster is created. | ||
:type no_wait: bool | ||
""" | ||
"""Create a new managed Kubernetes cluster.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this since it's already defined in help.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with the other 3 below...
@sptramer Please review. Thanks! |
|
||
helps['acs browse'] = """ | ||
type: command | ||
short-summary: Open a web browser to the dashboard for a container service's orchestrator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording; change to Show the dashboard for a service container's orchestrator in a web browser.
parameters: | ||
- name: --service-principal | ||
type: string | ||
short-summary: Service principal used for authentication to Azure APIs. If not specified, a new service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing definite article; The service principal used for...
- name: --service-principal | ||
type: string | ||
short-summary: Service principal used for authentication to Azure APIs. If not specified, a new service | ||
principal with contributor role is created and cached at {sp_cache} to be used by subsequent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing article; change to `...with the Contributor role...
type: string | ||
short-summary: Service principal used for authentication to Azure APIs. If not specified, a new service | ||
principal with contributor role is created and cached at {sp_cache} to be used by subsequent | ||
`az acs` commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies it is used by all ACS commands. This should be part of the user-controlled Azure configuration file. Is it instead used by commands for the created container service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually used by any az acs create
, az aks create
, or az aks install-connector
commands that follow.
parameters: | ||
- name: --service-principal | ||
type: string | ||
short-summary: Service principal used for authentication to Azure APIs. If not specified, a new service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short summary should be broken up. After the end of the first sentence should be turned into part of long-summary
. Keep in mind that both are shown with -h
and this will help with formatting and clarity.
@derekbekoe Is there something I am missing that would necessitate keeping this as a single short description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had tried it out that way, actually, but I balked just because the formatting of a long-summary:
field in a parameter looked unfamiliar to me:
$ az acs create --help
Command
az acs create: Create a new container service.
Arguments
--name -n [Required]: Name of the container service. You can configure the
default using `az configure --defaults acs=<name>`.
--resource-group -g [Required]: Name of resource group. You can configure the default
group using `az configure --defaults group=<name>`.
...
--service-principal : Service principal used for authentication to Azure APIs.
If not specified, a new service principal with contributor role is created and cached at
$HOME/.azure/acsServicePrincipal.json to be used by subsequent `az acs` commands.
--ssh-key-value : Configure all linux machines with the SSH RSA public key
string. Your key should include three parts, for example
'ssh-rsa AAAAB...snip...UcyupgH azureuser@linuxvm.
Default: ~/.ssh/id_rsa.pub.
But I agree with the intent of long-summary:
and I'll make this change.
|
||
helps['aks show'] = """ | ||
type: command | ||
short-summary: Show details of a managed Kubernetes cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to Show the details for a managed Kubernetes cluster.
helps['aks upgrade'] = """ | ||
type: command | ||
short-summary: Upgrade a managed Kubernetes cluster to a newer version. | ||
long-summary: "NOTE: Kubernetes may be unavailable during cluster upgrades." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove NOTE:
. Remove the unambiguous language of "may be" and also provide some information about how long a cluster upgrade may take.
helps['aks wait'] = """ | ||
type: command | ||
short-summary: Wait for a Kubernetes cluster to reach a desired state. | ||
short-summary: Wait for a managed Kubernetes cluster to reach a desired state. | ||
long-summary: If an operation on a cluster was interrupted or was started with `--no-wait`, use this command to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information is not included in acs wait
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The long-summary
does not include the following information:
- When the command returns if the condition is already met
- What status code is returned if it times out while waiting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns 0 if the condition is already met. But the command prints {}
and also returns 0 if it times out while waiting, which seems wrong to me. It's core CLI behavior we inherit here.
examples: | ||
- name: Wait for a cluster to be created. | ||
text: |- | ||
az aks create -g MyResourceGroup -n MyManagedCluster --no-wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not show the first command for any of these examples.
short-summary: Wait for a managed Kubernetes cluster to reach a desired state. | ||
long-summary: If an operation on a cluster was interrupted or was started with `--no-wait`, use this command to | ||
wait for it to complete. | ||
examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first three examples can be removed since they only show simple usage of the command. If waiting for a specific condition is tied to other aks
subcommands, that should be mentioned in the documentation for each argument.
9d9c84e
to
21112db
Compare
@sptramer thank you very much for the detailed review. I incorporated almost all of your comments directly. The ones I kicked down the road are:
Hopefully that puts this PR in the "good enough" category, but please let me know if you see anything else that should be changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better. Just one minor change to get rid of a typo and a reminder to create an issue for a lingering docs problem.
""" | ||
|
||
helps['acs wait'] = """ | ||
type: command | ||
short-summary: Wait for a container service to reach a desired state. | ||
long-summary: If an operation on a container service was interrupted or was started with `--no-wait`, use this | ||
command to wait for it to complete. Returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete Returns
""" | ||
|
||
helps['aks browse'] = """ | ||
type: command | ||
short-summary: Open a web browser to the dashboard for a managed Kubernetes cluster. | ||
short-summary: Show the dashboard for a Kubernetes cluster in a web browser. | ||
parameters: | ||
- name: --disable-browser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long-term I still think this needs more explanation or an example of why it would be used (since I guess it just sets up an HTTP tunnel?) but that can be a feature request/VSTS task/github issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #5114 to track that.
21112db
to
1fd2a53
Compare
Looks like another |
@mboersma Indeed... @tjprescott addressed them in #5100 so rebasing should fix most if not all of the errors. |
1fd2a53
to
aced724
Compare
Improves built-in help messages for
az aks
and someaz acs
commands. Moves many of them out of code into_help.py
as recommended.