Review feedback + help fixes

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_command_type.py

@@ -23,9 +23,6 @@
     extract_args_from_signature)
 
 
-# TODO: Enum choice lists
-
-
 _CLASS_NAME = re.compile(r"<(.*?)>")  # Strip model name from class docstring
 _UNDERSCORE_CASE = re.compile('(?!^)([A-Z]+)')  # Convert from CamelCase to underscore_case
 FLATTEN = 3  # The level of complex object namespace to flatten.
@@ -307,6 +304,7 @@ def compile_args(self):
         for name, details in self._arg_tree.items():
             if details['type'] == 'bool':
                 details['options']['action'] = 'store_true'
+                details['options']['help'] += ' True if flag present, otherwise default will be used.'
             elif details['type'].startswith('['):
                 details['options']['nargs'] = '+'
             elif details['type'] in ['iso-8601', 'rfc-1123']:
@@ -649,12 +647,16 @@ def _flatten_object(self, path, param_model, conflict_names=[]):  # pylint: disa
                     # We only expose a list arg if there's a validator for it
                     # This will fail for 2D arrays - though Batch doesn't have any yet
                     inner_type = details['type'][1:-1]
-                    if inner_type in BASIC_TYPES:  # TODO
-                        continue
+                    if inner_type in BASIC_TYPES:
+                        options['help'] += " Space separated values."
+                        self._resolve_conflict(
+                            param_attr, param_attr, path, options,
+                            details['type'], required_attrs, conflict_names)
                     else:
                         inner_type = operations_name(inner_type)
                         try:
                             validator = getattr(validators, inner_type + "_format")
+                            options['help'] += ' ' + validator.__doc__
                             options['type'] = validator
                             self._resolve_conflict(
                                 param_attr, param_attr, path, options,
@@ -664,6 +666,11 @@ def _flatten_object(self, path, param_model, conflict_names=[]):  # pylint: disa
                 else:
                     attr_model = _load_model(details['type'])
                     if not hasattr(attr_model, '_attribute_map'):  # Must be an enum
+                        values_index = options['help'].find(' Possible values include')
+                        if values_index >= 0:
+                            choices = options['help'][values_index + 25:].split(', ')
+                            options['choices'] = [c for c in choices if c != "'unmapped'"]
+                            options['help'] = options['help'][0:values_index]
                         self._resolve_conflict(param_attr, param_attr, path, options,
                                                details['type'], required_attrs, conflict_names)
                     else:
@@ -692,9 +699,9 @@ def _load_transformed_arguments(self, handler):
                     yield flattened_arg
                 param = 'json_file'
                 json_class = class_name(arg_type).split('.')[-1]
-                docstring = "A file containing the {} object in JSON format, " \
-                            "if this parameter is specified, all other parameters" \
-                            " are ignored.".format(json_class)
+                docstring = "A file containing the {} object in JSON format. " \
+                            "If this parameter is specified, all {} arguments" \
+                            " are ignored.".format(json_class, group_title(arg[0]))
                 yield (param, CliCommandArgument(param,
                                                  options_list=[arg_name(param)],
                                                  required=False,
@@ -737,15 +744,15 @@ def cli_batch_data_plane_command(name, operation, client_factory, transform=None
     group_name = 'Batch Account'
     command.cmd.add_argument('account_name', '--account-name', required=False, default=None,
                              validator=validators.validate_client_parameters, arg_group=group_name,
-                             help='Batch account name. Or set by environment variable: '
+                             help='Batch account name. Alternatively, set by environment variable: '
                              'AZURE_BATCH_ACCOUNT')
     command.cmd.add_argument('account_key', '--account-key', required=False, default=None,
                              arg_group=group_name,
                              help='Batch account key. Must be used in conjunction with Batch '
-                             'account name and endpoint. Or set by environment variable: '
+                             'account name and endpoint. Alternatively, set by environment variable: '
                              'AZURE_BATCH_ACCESS_KEY')
     command.cmd.add_argument('account_endpoint', '--account-endpoint', required=False, default=None,
                              arg_group=group_name,
-                             help='Batch service endpoint. Or set by environment variable: '
+                             help='Batch service endpoint. Alternatively, set by environment variable: '
                              'AZURE_BATCH_ENDPOINT')
     command_table[command.cmd.name] = command.cmd

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

@@ -27,6 +27,11 @@
     short-summary: Creates a new Batch account with the specified parameters.
 """
 
+helps['batch account set'] = """
+    type: command
+    short-summary: Updates the properties of the specified Batch account.
+"""
+
 helps['batch account autostorage-keys'] = """
     type: group
     short-summary: Commands to manage the access keys for the auto storage account configured for your Batch account.
@@ -42,11 +47,21 @@
     short-summary: Commands to manage your Batch applications.
 """
 
+helps['batch application set'] = """
+    type: command
+    short-summary: Updates the properties of the specified application.
+"""
+
 helps['batch application package'] = """
     type: group
     short-summary: Commands to manage your Batch application packages.
 """
 
+helps['batch application summary'] = """
+    type: group
+    short-summary: Commands to view a summary of your Batch application packages.
+"""
+
 helps['batch location'] = """
     type: group
     short-summary: Commands to manage Batch service options for a subscription at the region level.
@@ -59,14 +74,19 @@
 
 helps['batch certificate'] = """
     type: group
-    short-summary: Commands to manage your Batch certificate.
+    short-summary: Commands to manage your Batch certificates.
 """
 
 helps['batch task file'] = """
     type: group
     short-summary: Commands to manage your Batch task files.
 """
 
+helps['batch task file download'] = """
+    type: command
+    short-summary: Downloads the content of the specified task file.
+"""
+
 helps['batch node file'] = """
     type: group
     short-summary: Commands to manage your Batch compute node files.
@@ -79,7 +99,12 @@
 
 helps['batch job all-stats'] = """
     type: group
-    short-summary: Commands to manage all the jobs under your Batch account.
+    short-summary: Commands to manage the statistics of all the jobs under your Batch account
+"""
+
+helps['batch job all-stats show'] = """
+    type: command
+    short-summary: Gets lifetime summary statistics for all of the jobs in the specified account. Statistics are aggregated across all jobs that have ever existed in the account, from account creation to the last update time of the statistics.
 """
 
 helps['batch job prep-release-status'] = """
@@ -92,29 +117,34 @@
     short-summary: Commands to manage your Batch job schedules.
 """
 
-helps['batch node-user'] = """
+helps['batch node user'] = """
     type: group
     short-summary: Commands to manage your Batch compute node users.
 """
 
-helps['batch node-user create'] = """
+helps['batch node user create'] = """
     type: command
     short-summary: Adds a user account to the specified compute node.
 """
 
+helps['batch node user set'] = """
+    type: command
+    short-summary: Updates the properties of a user account on the specified compute node.
+"""
+
 helps['batch node'] = """
     type: group
     short-summary: Commands to manage your Batch compute nodes.
 """
 
 helps['batch node remote-login-settings'] = """
     type: group
-    short-summary: The remote login settings for a Batch compute node.
+    short-summary: Commands to manage the remote login settings for a Batch compute node.
 """
 
 helps['batch node remote-desktop'] = """
     type: group
-    short-summary: The remote desktop protocol for a Batch compute node.
+    short-summary: Commands to manage the remote desktop protocol for a Batch compute node.
 """
 
 helps['batch node scheduling'] = """
@@ -129,27 +159,32 @@
 
 helps['batch pool os'] = """
     type: group
-    short-summary: Commands to manage operationg system of your Batch pools.
+    short-summary: Commands to manage the operating system of your Batch pools.
 """
 
 helps['batch pool autoscale'] = """
     type: group
-    short-summary: Commands to manage autoscale of your Batch pools.
+    short-summary: Commands to manage automatic scaling of your Batch pools.
 """
 
 helps['batch pool all-stats'] = """
     type: group
-    short-summary: Commands to manage all the pools under your Batch account.
+    short-summary: Commands to manage statistics of all pools under your Batch account.
+"""
+
+helps['batch pool all-stats show'] = """
+    type: command
+    short-summary: Gets lifetime summary statistics for all of the pools in the specified account. Statistics are aggregated across all pools that have ever existed in the account, from account creation to the last update time of the statistics.
 """
 
 helps['batch pool usage-metrics'] = """
     type: group
-    short-summary: Commands to manage usage metrics of your Batch pools'.
+    short-summary: Commands to manage usage metrics of your Batch pools.
 """
 
 helps['batch pool node-agent-skus'] = """
     type: group
-    short-summary: Commands to manage node agent skus of IaaS pools'.
+    short-summary: Commands to manage node agent skus of pools using a Virtual Machine Configuration.
 """
 
 helps['batch task'] = """
@@ -179,12 +214,12 @@
 
 helps['batch pool set'] = """
     type: command
-    short-summary: Patch the properties of the specified Batch pool.
+    short-summary: Updates the properties of the specified pool. Properties that are not specified remain unchanged.
 """
 
 helps['batch pool reset'] = """
     type: command
-    short-summary: Update the properties of the specified Batch pool.
+    short-summary: Updates the properties of the specified pool. All updatable properties are replaced with the values specified or cleared if unspecified.
 """
 
 helps['batch pool resize'] = """

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_params.py

@@ -25,17 +25,21 @@
 
 # PARAMETER REGISTRATIONS
 
-register_cli_argument('batch', 'resource_group_name', resource_group_name_type, completer=None, validator=None)
+register_cli_argument('batch', 'resource_group_name', resource_group_name_type, help='Name of the resource group', completer=None, validator=None)
 register_cli_argument('batch account', 'account_name', batch_name_type, options_list=('--name', '-n'))
-register_cli_argument('batch account create', 'location', location_type)
-register_cli_argument('batch account create', 'tags', tags_type)
-register_extra_cli_argument('batch account create', 'storage_account', help='The storage account name or resource id to be used for auto storage account.', validator=storage_account_id)
+register_cli_argument('batch account create', 'location', location_type, help='The region in which to create the account.')
+register_cli_argument('batch account create', 'tags', tags_type, help="Space separated tags in 'key[=value]' format.")
+register_extra_cli_argument('batch account create', 'storage_account', help='The storage account name or resource id to be used for auto storage.', validator=storage_account_id)
 register_cli_argument('batch account set', 'tags', tags_type)
-register_extra_cli_argument('batch account set', 'storage_account', help='The storage account name or resource id to be used for auto storage account.', validator=storage_account_id)
+register_extra_cli_argument('batch account set', 'storage_account', help='The storage account name or resource id to be used for auto storage.', validator=storage_account_id)
 register_cli_argument('batch account keys renew', 'key_name', **enum_choice_list(AccountKeyType))
-register_cli_argument('batch application', 'account_name', batch_name_type, options_list=('--name', '-n'), validator=application_enabled)
+register_cli_argument('batch application set', 'application_id', options_list=('--application-id',), help="The ID of the application.")
+register_cli_argument('batch application set', 'allow_updates', options_list=('--allow-updates',), action="store_true", help="A value indicating whether packages within the application may be overwritten using the same version string. True if flag present.")
+register_cli_argument('batch application create', 'allow_updates', options_list=('--allow-updates',), action="store_true", help="A value indicating whether packages within the application may be overwritten using the same version string. True if flag present.")
 register_cli_argument('batch application package create', 'package_file', type=file_type, help='The path of the application package in zip format', completer=FilesCompleter())
-register_cli_argument('batch location quotas show', 'location_name', location_type)
+register_cli_argument('batch application package create', 'application_id', options_list=('--application-id',), help="The ID of the application.")
+register_cli_argument('batch application package create', 'version', options_list=('--version',), help="The version of the application.")
+register_cli_argument('batch location quotas show', 'location_name', location_type, help='The region from which to display the Batch service quotas.')
 
 for command in ['list', 'show', 'create', 'set', 'delete', 'package']:
     register_cli_argument('batch application {}'.format(command), 'account_name', batch_name_type, options_list=('--name', '-n'), validator=application_enabled)
@@ -72,8 +76,8 @@
 for item in ['batch certificate delete', 'batch certificate create', 'batch pool resize', 'batch pool reset', 'batch job list', 'batch task create']:
     register_extra_cli_argument(item, 'account_name', arg_group='Batch Account',
                                 validator=validate_client_parameters,
-                                help='The Batch account name. Or set by environment variable: AZURE_BATCH_ACCOUNT')
+                                help='The Batch account name. Alternatively, set by environment variable: AZURE_BATCH_ACCOUNT')
     register_extra_cli_argument(item, 'account_key', arg_group='Batch Account',
-                                help='The Batch account key. Or set by environment variable: AZURE_BATCH_ACCESS_KEY')
+                                help='The Batch account key. Alternatively, set by environment variable: AZURE_BATCH_ACCESS_KEY')
     register_extra_cli_argument(item, 'account_endpoint', arg_group='Batch Account',
-                                help='Batch service endpoint. Or set by environment variable: AZURE_BATCH_ENDPOINT')
+                                help='Batch service endpoint. Alternatively, set by environment variable: AZURE_BATCH_ENDPOINT')

src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/_validators.py

@@ -37,7 +37,7 @@ def duration_format(value):
 
 
 def metadata_item_format(value):
-    """Validate listed metadata arguments"""
+    """Space separated values in 'key=value' format."""
     try:
         data_name, data_value = value.split('=')
     except ValueError:
@@ -49,7 +49,7 @@ def metadata_item_format(value):
 
 
 def environment_setting_format(value):
-    """Validate listed enviroment settings arguments"""
+    """Space separated values in 'key=value' format."""
     try:
         env_name, env_value = value.split('=')
     except ValueError:
@@ -61,7 +61,7 @@ def environment_setting_format(value):
 
 
 def application_package_reference_format(value):
-    """Validate listed application package reference arguments"""
+    """Space separated application IDs with optional version in 'id[#version]' format."""
     app_reference = value.split('#', 1)
     package = {'application_id': app_reference[0]}
     try:
@@ -72,7 +72,7 @@ def application_package_reference_format(value):
 
 
 def certificate_reference_format(value):
-    """Validate listed certificate reference arguments"""
+    """Space separated certificate thumbprints."""
     cert = {'thumbprint': value, 'thumbprint_algorithm': 'sha1'}
     return cert
 
@@ -90,18 +90,16 @@ def storage_account_id(namespace):
     from azure.mgmt.storage import StorageManagementClient
     from azure.cli.core.commands.client_factory import get_mgmt_service_client
 
-    if (namespace.storage_account
-            and not
-            ('/providers/Microsoft.ClassicStorage/storageAccounts/' in namespace.storage_account
-             or
-             '/providers/Microsoft.Storage/storageAccounts/' in namespace.storage_account)):
-        storage_client = get_mgmt_service_client(StorageManagementClient)
-        acc = storage_client.storage_accounts.get_properties(namespace.resource_group_name,
-                                                             namespace.storage_account)
-        if not acc:
-            raise ValueError("Batch account named '{}' not found in the resource group '{}'.".
-                             format(namespace.storage_account, namespace.resource_group_name))
-        namespace.storage_account = acc.id  # pylint: disable=no-member
+    if (namespace.storage_account and not
+        ('/providers/Microsoft.ClassicStorage/storageAccounts/' in namespace.storage_account or
+         '/providers/Microsoft.Storage/storageAccounts/' in namespace.storage_account)):
+            storage_client = get_mgmt_service_client(StorageManagementClient)
+            acc = storage_client.storage_accounts.get_properties(namespace.resource_group_name,
+                                                                 namespace.storage_account)
+            if not acc:
+                raise ValueError("Batch account named '{}' not found in the resource group '{}'.".
+                                 format(namespace.storage_account, namespace.resource_group_name))
+            namespace.storage_account = acc.id  # pylint: disable=no-member
 
 
 def application_enabled(namespace):
@@ -234,7 +232,7 @@ def validate_pool_settings(ns, parser):
 
         paas_sizes = ['small', 'medium', 'large', 'extralarge']
         if ns.vm_size and ns.vm_size.lower() in paas_sizes and not ns.os_family:
-            message = ("The selected VM size in incompatible with Virtual Machine Configuration. "
-                       "Please swap for the IaaS equivalent: Standard_A1 (small), Standard_A2 "
+            message = ("The selected VM size is incompatible with Virtual Machine Configuration. "
+                       "Please swap for the equivalent: Standard_A1 (small), Standard_A2 "
                        "(medium), Standard_A3 (large), or Standard_A4 (extra large).")
             raise ValueError(message)