[AKS] Knack - improve help messages #5078

mboersma · 2017-12-11T16:55:50Z

Improves built-in help messages for az aks and some az acs commands. Moves many of them out of code into _help.py as recommended.

azuresdkci · 2017-12-11T16:56:08Z

View a preview at https://prompt.ws/r/Azure/azure-cli/5078
This is an experimental preview for @microsoft.com users.
(It may take a minute or two for your instance to be ready)
Email feedback to 'azfeedback' with subject 'Prompt Feedback'.

derekbekoe

Can remove the doc strings from methods in custom.py.

derekbekoe · 2017-12-12T18:09:49Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

-     kubectl port-forward
-    :type disable_browser: bool
-    """
+    """Open a web browser to the dashboard for a managed Kubernetes cluster."""


You can remove this since it's already defined in help.py.

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.

Okay that's fine. Up to you.

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.

Makes sense to me. I'll remove them.

derekbekoe · 2017-12-12T18:09:59Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

-     until the managed cluster is created.
-    :type no_wait: bool
-    """
+    """Create a new managed Kubernetes cluster."""


You can remove this since it's already defined in help.py.

Same with the other 3 below...

derekbekoe · 2017-12-12T19:29:45Z

@sptramer Please review. Thanks!

sptramer · 2017-12-12T20:45:36Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+
+helps['acs browse'] = """
+    type: command
+    short-summary: Open a web browser to the dashboard for a container service's orchestrator.


Wording; change to Show the dashboard for a service container's orchestrator in a web browser.

sptramer · 2017-12-12T20:46:16Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+    parameters:
+        - name: --service-principal
+          type: string
+          short-summary: Service principal used for authentication to Azure APIs. If not specified, a new service


Missing definite article; The service principal used for...

sptramer · 2017-12-12T20:46:58Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+        - 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


Missing article; change to `...with the Contributor role...

sptramer · 2017-12-12T20:48:06Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+          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.


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?

It's actually used by any az acs create, az aks create, or az aks install-connector commands that follow.

sptramer · 2017-12-12T20:49:16Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+    parameters:
+        - name: --service-principal
+          type: string
+          short-summary: Service principal used for authentication to Azure APIs. If not specified, a new service


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?

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.

sptramer · 2017-12-12T23:03:01Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+
+helps['aks show'] = """
+    type: command
+    short-summary: Show details of a managed Kubernetes cluster.


Change to Show the details for a managed Kubernetes cluster.

sptramer · 2017-12-12T23:16:12Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+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."


Remove NOTE:. Remove the unambiguous language of "may be" and also provide some information about how long a cluster upgrade may take.

sptramer · 2017-12-12T23:21:55Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

 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


This information is not included in acs wait.

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

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.

sptramer · 2017-12-12T23:22:16Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+    examples:
+        - name: Wait for a cluster to be created.
+          text: |-
+            az aks create -g MyResourceGroup -n MyManagedCluster --no-wait


Do not show the first command for any of these examples.

sptramer · 2017-12-12T23:23:51Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+    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:


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.

mboersma · 2017-12-13T20:28:45Z

@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:

didn't add The to the start of each argument's short-summary: because that's inconsistent with the global arguments
didn't add an estimate for how long az aks upgrade would take, because it's not easy to be specific yet
didn't add return code information to az [acs|aks] wait because I'm not sure if the behavior is correct

Hopefully that puts this PR in the "good enough" category, but please let me know if you see anything else that should be changed.

sptramer

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.

sptramer · 2017-12-14T20:55:11Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

 """

 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


Delete Returns

sptramer · 2017-12-14T23:33:00Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

 """

 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


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.

I created #5114 to track that.

mboersma · 2017-12-15T23:05:51Z

Looks like another pylint update with new rules? I can try to clean those up unless someone else is already on it...

derekbekoe · 2017-12-15T23:09:52Z

@mboersma Indeed... @tjprescott addressed them in #5100 so rebasing should fix most if not all of the errors.

derekbekoe approved these changes Dec 12, 2017

View reviewed changes

derekbekoe suggested changes Dec 12, 2017

View reviewed changes

derekbekoe approved these changes Dec 12, 2017

View reviewed changes

derekbekoe added this to the Sprint 28 - Final 2017 Release milestone Dec 12, 2017

derekbekoe requested a review from sptramer December 12, 2017 19:29

mboersma changed the title ~~[AKS] improve help messages~~ [AKS] Knack - improve help messages Dec 12, 2017

sptramer suggested changes Dec 12, 2017

View reviewed changes

mboersma force-pushed the acs-update-help-strings branch 3 times, most recently from 9d9c84e to 21112db Compare December 13, 2017 20:27

mboersma mentioned this pull request Dec 13, 2017

[AKS] improve help messages #5095

Closed

sptramer suggested changes Dec 14, 2017

View reviewed changes

mboersma mentioned this pull request Dec 15, 2017

Revisit --disable-browser argument to [acs|aks] browse #5114

Closed

mboersma force-pushed the acs-update-help-strings branch from 21112db to 1fd2a53 Compare December 15, 2017 16:48

mboersma added 4 commits December 15, 2017 16:13

[AKS] improve help messages

9801384

Remove redundant docstrings

0cbc89c

Updated help text to incorporate review comments.

6e9c210

Fix a typo in aks wait command help.

aced724

mboersma force-pushed the acs-update-help-strings branch from 1fd2a53 to aced724 Compare December 15, 2017 23:14

tjprescott merged commit 23728f4 into Azure:KnackConversion Dec 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AKS] Knack - improve help messages #5078

[AKS] Knack - improve help messages #5078

mboersma commented Dec 11, 2017

azuresdkci commented Dec 11, 2017

derekbekoe left a comment

derekbekoe Dec 12, 2017

mboersma Dec 12, 2017

derekbekoe Dec 12, 2017

tjprescott Dec 12, 2017

mboersma Dec 12, 2017

derekbekoe Dec 12, 2017

derekbekoe Dec 12, 2017

derekbekoe commented Dec 12, 2017

sptramer Dec 12, 2017

sptramer Dec 12, 2017

sptramer Dec 12, 2017

sptramer Dec 12, 2017

mboersma Dec 13, 2017

sptramer Dec 12, 2017

mboersma Dec 13, 2017

sptramer Dec 12, 2017

sptramer Dec 12, 2017

sptramer Dec 12, 2017

sptramer Dec 12, 2017

mboersma Dec 13, 2017

sptramer Dec 12, 2017

sptramer Dec 12, 2017

mboersma commented Dec 13, 2017 •

edited

Loading

sptramer left a comment

sptramer Dec 14, 2017

sptramer Dec 14, 2017

mboersma Dec 15, 2017

mboersma commented Dec 15, 2017

derekbekoe commented Dec 15, 2017

[AKS] Knack - improve help messages #5078

[AKS] Knack - improve help messages #5078

Conversation

mboersma commented Dec 11, 2017

azuresdkci commented Dec 11, 2017

derekbekoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mboersma commented Dec 13, 2017 • edited Loading

sptramer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mboersma commented Dec 15, 2017

derekbekoe commented Dec 15, 2017

mboersma commented Dec 13, 2017 •

edited

Loading