--ids parameter fixes

src/azure-cli-core/HISTORY.rst

@@ -4,7 +4,8 @@ Release History
 ===============
 2.0.48
 ++++++
-* Minor fixes
+* Fix issue with `--ids` where `--subscription` would take precedence over the subscription in `--ids`.
+  Adding explicit warnings when name parameters would be ignored by use of `--ids`.
 
 2.0.47
 ++++++

src/azure-cli-core/azure/cli/core/__init__.py

@@ -33,12 +33,12 @@ class AzCli(CLI):
     def __init__(self, **kwargs):
         super(AzCli, self).__init__(**kwargs)
 
-        from azure.cli.core.commands.arm import add_id_parameters, register_global_subscription_parameter
+        from azure.cli.core.commands.arm import (
+            register_ids_argument, register_global_subscription_argument)
         from azure.cli.core.cloud import get_active_cloud
         from azure.cli.core.extensions import register_extensions
         from azure.cli.core._session import ACCOUNT, CONFIG, SESSION
 
-        import knack.events as events
         from knack.util import ensure_dir
 
         self.data['headers'] = {}
@@ -56,8 +56,8 @@ def __init__(self, **kwargs):
         logger.debug('Current cloud config:\n%s', str(self.cloud.name))
 
         register_extensions(self)
-        self.register_event(events.EVENT_INVOKER_POST_CMD_TBL_CREATE, add_id_parameters)
-        register_global_subscription_parameter(self)
+        register_global_subscription_argument(self)
+        register_ids_argument(self)  # global subscription must be registered first!
 
         self.progress_controller = None
 

src/azure-cli-core/azure/cli/core/commands/__init__.py

@@ -120,6 +120,7 @@ def __init__(self, loader, name, handler, description=None, table_transformer=No
 
     def _resolve_default_value_from_cfg_file(self, arg, overrides):
         from azure.cli.core._config import DEFAULTS_SECTION
+        from azure.cli.core.commands.validators import DefaultStr
 
         if not hasattr(arg.type, 'required_tooling'):
             required = arg.type.settings.get('required', False)
@@ -130,11 +131,10 @@ def _resolve_default_value_from_cfg_file(self, arg, overrides):
             # same blunt mechanism like we handled id-parts, for create command, no name default
             if self.name.split()[-1] == 'create' and overrides.settings.get('metavar', None) == 'NAME':
                 return
-            setattr(arg.type, 'configured_default_applied', True)
             config_value = self.cli_ctx.config.get(DEFAULTS_SECTION, def_config, None)
             if config_value:
                 logger.info("Configured default '%s' for arg %s", config_value, arg.name)
-                overrides.settings['default'] = config_value
+                overrides.settings['default'] = DefaultStr(config_value)
                 overrides.settings['required'] = False
 
     def load_arguments(self):
@@ -296,6 +296,9 @@ def execute(self, args):
 
             self.cli_ctx.data['command'] = expanded_arg.command
 
+            if hasattr(expanded_arg, '_subscription'):
+                self.cli_ctx.data['subscription_id'] = expanded_arg._subscription  # pylint: disable=protected-access
+
             self._validation(expanded_arg)
 
             params = self._filter_params(expanded_arg)

src/azure-cli-core/azure/cli/core/commands/arm.py

@@ -172,128 +172,128 @@ def resource_exists(cli_ctx, resource_group, name, namespace, type, **_):  # pyl
     return existing
 
 
-def add_id_parameters(_, **kwargs):  # pylint: disable=unused-argument
+def register_ids_argument(cli_ctx):
 
-    command_table = kwargs.get('commands_loader').command_table
+    from knack import events
+    from msrestazure.tools import parse_resource_id, is_valid_resource_id
+    import os
 
-    if not command_table:
-        return
+    ids_metadata = {}
 
-    def split_action(arguments, deprecate_info):
-        class SplitAction(argparse.Action):  # pylint: disable=too-few-public-methods
-            def __call__(self, parser, namespace, values, option_string=None):
-                ''' The SplitAction will take the given ID parameter and spread the parsed
-                parts of the id into the individual backing fields.
-
-                Since the id value is expected to be of type `IterateValue`, all the backing
-                (dest) fields will also be of type `IterateValue`
-                '''
-                from msrestazure.tools import parse_resource_id
-                import os
-
-                if isinstance(values, str):
-                    values = [values]
-                expanded_values = []
-                for val in values:
-                    try:
-                        # support piping values from JSON. Does not require use of --query
-                        json_vals = json.loads(val)
-                        if not isinstance(json_vals, list):
-                            json_vals = [json_vals]
-                        for json_val in json_vals:
-                            if 'id' in json_val:
-                                expanded_values += [json_val['id']]
-                    except ValueError:
-                        # supports piping of --ids to the command when using TSV. Requires use of --query
-                        expanded_values = expanded_values + val.split(os.linesep)
-                try:
-                    for value in expanded_values:
-                        parts = parse_resource_id(value)
-                        for arg in [arg for arg in arguments.values() if arg.type.settings.get('id_part')]:
-                            self.set_argument_value(namespace, arg, parts)
-                except Exception as ex:
-                    raise ValueError(ex)
-
-                if deprecate_info:
-                    if not hasattr(namespace, '_argument_deprecations'):
-                        setattr(namespace, '_argument_deprecations', [deprecate_info])
-                    else:
-                        namespace._argument_deprecations.append(deprecate_info)  # pylint: disable=protected-access
-
-            @staticmethod
-            def set_argument_value(namespace, arg, parts):
-
-                existing_values = getattr(namespace, arg.name, None)
-                if existing_values is None:
-                    existing_values = IterateValue()
-                    existing_values.append(parts.get(arg.type.settings['id_part'], None))
-                else:
-                    if isinstance(existing_values, str):
-                        if not getattr(arg.type, 'configured_default_applied', None):
-                            logger.warning(
-                                "Property '%s=%s' being overriden by value '%s' from IDs parameter.",
-                                arg.name, existing_values, parts[arg.type.settings['id_part']]
-                            )
-                        existing_values = IterateValue()
-                    existing_values.append(parts.get(arg.type.settings['id_part']))
-                setattr(namespace, arg.name, existing_values)
-
-        return SplitAction
-
-    def command_loaded_handler(command):
-        id_parts = [arg.type.settings['id_part'] for arg in command.arguments.values()
-                    if arg.type.settings.get('id_part')]
-        if 'name' not in id_parts and 'resource_name' not in id_parts:
-            # Only commands with a resource name are candidates for an id parameter
-            return
-        if command.name.split()[-1] == 'create':
-            # Somewhat blunt hammer, but any create commands will not have an automatic id
-            # parameter
+    def add_ids_arguments(_, **kwargs):  # pylint: disable=unused-argument
+
+        command_table = kwargs.get('commands_loader').command_table
+
+        if not command_table:
             return
 
-        required_arguments = []
-        optional_arguments = []
-        for arg in [argument for argument in command.arguments.values() if argument.type.settings.get('id_part')]:
-            if arg.options.get('required', False):
-                required_arguments.append(arg)
-            else:
-                optional_arguments.append(arg)
-            arg.required = False
+        for command in command_table.values():
+
+            # Somewhat blunt hammer, but any create commands will not have an automatic id parameter
+            if command.name.split()[-1] == 'create':
+                continue
+
+            # Only commands with a resource name are candidates for an id parameter
+            id_parts = [a.type.settings.get('id_part') for a in command.arguments.values()]
+            if 'name' not in id_parts and 'resource_name' not in id_parts:
+                continue
 
-        def required_values_validator(namespace):
+            group_name = 'Resource Id'
 
-            errors = [arg for arg in required_arguments
-                      if getattr(namespace, arg.name, None) is None]
+            # determine which arguments are required and optional and store in ids_metadata
+            ids_metadata[command.name] = {'required': [], 'optional': []}
+            for arg in [a for a in command.arguments.values() if a.type.settings.get('id_part')]:
+                if arg.options.get('required', False):
+                    ids_metadata[command.name]['required'].append(arg.name)
+                else:
+                    ids_metadata[command.name]['optional'].append(arg.name)
+                arg.required = False
+                arg.arg_group = group_name
+
+            # retrieve existing `ids` arg if it exists
+            id_arg = command.loader.argument_registry.arguments[command.name].get('ids', None)
+            deprecate_info = id_arg.settings.get('deprecate_info', None) if id_arg else None
+            id_kwargs = {
+                'metavar': 'ID',
+                'help': "One or more resource IDs (space-delimited). If provided, "
+                        "no other 'Resource Id' arguments should be specified.",
+                'dest': 'ids' if id_arg else '_ids',
+                'deprecate_info': deprecate_info,
+                'nargs': '+',
+                'arg_group': group_name
+            }
+            command.add_argument('ids', '--ids', **id_kwargs)
+
+    def parse_ids_arguments(_, command, args):
+
+        namespace = args
+        cmd = namespace._cmd  # pylint: disable=protected-access
+
+        # some commands have custom IDs and parsing. This will not work for that.
+        if not ids_metadata.get(command, None):
+            return
 
+        ids = getattr(namespace, 'ids', getattr(namespace, '_ids', None))
+        required_args = [cmd.arguments[x] for x in ids_metadata[command]['required']]
+        optional_args = [cmd.arguments[x] for x in ids_metadata[command]['optional']]
+        combined_args = required_args + optional_args
+
+        if not ids:
+            # ensure the required parameters are provided if --ids is not
+            errors = [arg for arg in required_args if getattr(namespace, arg.name, None) is None]
             if errors:
                 missing_required = ' '.join((arg.options_list[0] for arg in errors))
                 raise ValueError('({} | {}) are required'.format(missing_required, '--ids'))
+            return
 
-        group_name = 'Resource Id'
-        for key, arg in command.arguments.items():
-            if command.arguments[key].type.settings.get('id_part'):
-                command.arguments[key].arg_group = group_name
-
-        id_arg = command.loader.argument_registry.arguments[command.name].get('ids', None)
-        deprecate_info = id_arg.settings.get('deprecate_info', None) if id_arg else None
-        id_kwargs = {
-            'metavar': 'RESOURCE_ID',
-            'help': "One or more resource IDs (space-delimited). If provided, "
-                    "no other 'Resource Id' arguments should be specified.",
-            'dest': argparse.SUPPRESS,
-            'action': split_action(command.arguments, deprecate_info),
-            'deprecate_info': deprecate_info,
-            'nargs': '+',
-            'validator': required_values_validator,
-            'arg_group': group_name
-        }
-        command.add_argument('ids', '--ids', **id_kwargs)
+        # show warning if names are used in conjunction with --ids
+        other_values = {arg.name: {'arg': arg, 'value': getattr(namespace, arg.name, None)}
+                        for arg in combined_args}
+        for _, data in other_values.items():
+            if data['value'] and not getattr(data['value'], 'is_default', None):
+                logger.warning("option '%s' will be ignored due to use of '--ids'.",
+                               data['arg'].type.settings['options_list'][0])
+
+        # create the empty lists, overwriting any values that may already be there
+        for arg in combined_args:
+            setattr(namespace, arg.name, IterateValue())
+
+        # expand the IDs into the relevant fields
+        full_id_list = []
+        for val in ids:
+            try:
+                # support piping values from JSON. Does not require use of --query
+                json_vals = json.loads(val)
+                if not isinstance(json_vals, list):
+                    json_vals = [json_vals]
+                for json_val in json_vals:
+                    if 'id' in json_val:
+                        full_id_list += [json_val['id']]
+            except ValueError:
+                # supports piping of --ids to the command when using TSV. Requires use of --query
+                full_id_list = full_id_list + val.split(os.linesep)
+
+        for val in full_id_list:
+            if not is_valid_resource_id(val):
+                raise CLIError('invalid resource ID: {}'.format(val))
+            # place the ID parts into the correct property lists
+            parts = parse_resource_id(val)
+            for arg in combined_args:
+                getattr(namespace, arg.name).append(parts[arg.type.settings['id_part']])
+
+        # support deprecating --ids
+        deprecate_info = cmd.arguments['ids'].type.settings.get('deprecate_info')
+        if deprecate_info:
+            if not hasattr(namespace, '_argument_deprecations'):
+                setattr(namespace, '_argument_deprecations', [deprecate_info])
+            else:
+                namespace._argument_deprecations.append(deprecate_info)  # pylint: disable=protected-access
 
-    for command in command_table.values():
-        command_loaded_handler(command)
+    cli_ctx.register_event(events.EVENT_INVOKER_POST_CMD_TBL_CREATE, add_ids_arguments)
+    cli_ctx.register_event(events.EVENT_INVOKER_POST_PARSE_ARGS, parse_ids_arguments)
 
 
-def register_global_subscription_parameter(cli_ctx):
+def register_global_subscription_argument(cli_ctx):
 
     import knack.events as events
 
@@ -307,21 +307,14 @@ def add_subscription_parameter(_, **kwargs):
                     'using `az account set -s NAME_OR_ID`',
             'completer': get_subscription_id_list,
             'arg_group': 'Global',
-            'configured_default': 'subscription'
+            'configured_default': 'subscription',
+            'id_part': 'subscription'
         }
         for _, cmd in cmd_tbl.items():
             if 'subscription' not in cmd.arguments:
                 cmd.add_argument('_subscription', '--subscription', **subscription_kwargs)
 
-    def parse_subscription_parameter(cli_ctx, args, **kwargs):  # pylint: disable=unused-argument
-        subscription = getattr(args, '_subscription', None)
-        if subscription:
-            from azure.cli.core._profile import Profile
-            subscription_id = Profile(cli_ctx=cli_ctx).get_subscription_id(subscription)
-            cli_ctx.data['subscription_id'] = subscription_id
-
     cli_ctx.register_event(events.EVENT_INVOKER_POST_CMD_TBL_CREATE, add_subscription_parameter)
-    cli_ctx.register_event(events.EVENT_INVOKER_POST_PARSE_ARGS, parse_subscription_parameter)
 
 
 add_usage = '--add property.listProperty <key=value, string or JSON string>'

src/azure-cli-core/azure/cli/core/commands/client_factory.py

@@ -171,11 +171,9 @@ def get_data_service_client(cli_ctx, service_type, account_name, account_key, co
 
 def get_subscription_id(cli_ctx):
     from azure.cli.core._profile import Profile
-    if 'subscription_id' in cli_ctx.data:
-        subscription_id = cli_ctx.data['subscription_id']
-    else:
-        subscription_id = Profile(cli_ctx=cli_ctx).get_subscription_id()
-    return subscription_id
+    if not cli_ctx.data.get('subscription_id'):
+        cli_ctx.data['subscription_id'] = Profile(cli_ctx=cli_ctx).get_subscription_id()
+    return cli_ctx.data['subscription_id']
 
 
 def _get_add_headers_callback(cli_ctx):

src/azure-cli-core/azure/cli/core/file_util.py

@@ -8,7 +8,6 @@
 from knack.help import GroupHelpFile
 
 from azure.cli.core._help import CliCommandHelpFile
-from azure.cli.core.commands.arm import add_id_parameters
 
 
 def get_all_help(cli_ctx):
@@ -40,14 +39,20 @@ def get_all_help(cli_ctx):
 
 
 def create_invoker_and_load_cmds_and_args(cli_ctx):
+    from knack import events
+    from azure.cli.core.commands.arm import register_global_subscription_argument, register_ids_argument
+
     invoker = cli_ctx.invocation_cls(cli_ctx=cli_ctx, commands_loader_cls=cli_ctx.commands_loader_cls,
                                      parser_cls=cli_ctx.parser_cls, help_cls=cli_ctx.help_cls)
     cli_ctx.invocation = invoker
     invoker.commands_loader.load_command_table(None)
     for command in invoker.commands_loader.command_table:
         invoker.commands_loader.load_arguments(command)
     invoker.parser.load_command_table(invoker.commands_loader)
-    add_id_parameters(None, commands_loader=invoker.commands_loader)
+
+    register_global_subscription_argument(cli_ctx)
+    register_ids_argument(cli_ctx)  # global subscription must be registered first!
+    cli_ctx.raise_event(events.EVENT_INVOKER_POST_CMD_TBL_CREATE, commands_loader=invoker.commands_loader)
 
 
 def _store_parsers(parser, parser_keys, parser_values, sub_parser_keys, sub_parser_values):

src/azure-cli-core/azure/cli/core/tests/test_help.py

@@ -25,7 +25,7 @@ def tearDownClass(cls):
         logging.shutdown()
 
     def test_help_loads(self):
-        from azure.cli.core.commands.arm import add_id_parameters
+        from azure.cli.core.commands.arm import register_global_subscription_argument, register_ids_argument
         import knack.events as events
 
         cli = DummyCli()
@@ -44,7 +44,8 @@ def test_help_loads(self):
                 cmd_tbl[cmd].loader.load_arguments(cmd)
             except KeyError:
                 pass
-        cli.register_event(events.EVENT_INVOKER_POST_CMD_TBL_CREATE, add_id_parameters)
+        cli.register_event(events.EVENT_INVOKER_POST_CMD_TBL_CREATE, register_global_subscription_argument)
+        cli.register_event(events.EVENT_INVOKER_POST_CMD_TBL_CREATE, register_ids_argument)
         cli.raise_event(events.EVENT_INVOKER_CMD_TBL_LOADED, command_table=cmd_tbl)
         cli.invocation.parser.load_command_table(cli.invocation.commands_loader)
         _store_parsers(cli.invocation.parser, parser_dict)

src/azure-cli-core/setup.py

@@ -59,7 +59,7 @@
     'colorama>=0.3.9',
     'humanfriendly>=4.7',
     'jmespath',
-    'knack==0.4.3',
+    'knack==0.4.4',
     'msrest>=0.4.4',
     'msrestazure>=0.4.25',
     'paramiko>=2.0.8',