resource: support resource id for show/delete/tag #1280

yugangw-msft · 2016-11-09T21:19:54Z

No description provided.

yugangw-msft · 2016-11-10T05:10:16Z

@tjprescott, could you please take a look?

tjprescott

A few questions and some minor tweaks requested, but otherwise LGTM (and no I don't know why I'm reviewing PRs at 5 am)

tjprescott · 2016-11-10T13:31:54Z

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

-
-register_cli_argument('resource', 'resource_type', help='The resource type in <namespace>/<type> format.', type=validate_resource_type, validator=resolve_resource_parameters)
-register_cli_argument('resource', 'parent_resource_path', help='The parent resource type in <type>/<name> format.', type=validate_parent, required=False, options_list=('--parent',))
+register_cli_argument('resource', 'resource_id', options_list=('--id',), help='resource id')


Should we reasonably expect the --ids semantic (of looping through multiple ids) to work here? If it can be done easily, I would suggest that we do. If it can't, then I wouldn't worry about it for this PR.

Thought about it, the ids will have to pair with api-versions, and have a many to many association between 2 value groups won't make command easier to use. So let us punt for now

tjprescott · 2016-11-10T13:34:06Z

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

+                      help="resource namespaces like 'Microsoft.Network'. You can also specify it through --resource-type parameter")
+register_cli_argument('resource', 'resource_type', help="types such as 'virtualNetworks', or optional include namespace such as 'Microsoft.Network/virtualNetworks'")
+register_cli_argument('resource', 'parent_resource_path', required=False, options_list=('--parent',),
+                      help="path in formats of '<parent-type>/<parent-name>' or '<grandparent-type>/<grandparent-name>/<parent-type>/<parent-name>'")


Here it might be better to give examples such as a SQL server and database path for the grandparent case and pretty much any normal one for the parent-type/parent-name case.

Sql Server and database is still parent-child relationship. I have added a concrete example.
For grand parent relationship, I searched across cli, looks like we only have one case on app gateway url-path-map rules. Do you have related resource types detail?

Ah, no, you're right.

tjprescott · 2016-11-10T13:35:21Z

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

+register_cli_argument('resource', 'resource_id', options_list=('--id',), help='resource id')
+register_cli_argument('resource', 'resource_provider_namespace', options_list=('--namespace',), completer=get_providers_completion_list,
+                      help="resource namespaces like 'Microsoft.Network'. You can also specify it through --resource-type parameter")
+register_cli_argument('resource', 'resource_type', help="types such as 'virtualNetworks', or optional include namespace such as 'Microsoft.Network/virtualNetworks'")


Should there be a completer on this? Are we maintaining the ability to optionally include the namespace here to avoid breaking changes?

Yes, we are still maintaining this ability. This one is a redundant, so I have removed. We have another similar registration at a few lines above which does have a completer

tjprescott · 2016-11-10T13:38:06Z

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

-            'API version is required and could not be resolved for resource {}/{}'
-            .format(resource_type.namespace, resource_type.type))
-
-def resolve_resource_parameters(namespace):


If we maintain the ability to specify resource_type as 'provider/type' then where do we do that sniffing/parsing/checking?

Yep, we still maintain the logic, check out the new code in _ResourceUtils.__init__

tjprescott · 2016-11-10T13:39:59Z

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

-            api_version,
-            parameters)
-    except CloudError as ex:
-        # TODO: Remove workaround once Swagger and SDK fix is implemented (#120123723)


I'm assuming this block of logic has been moved and not removed. Is this workaround still needed?

Yep, this is the existing code, but agree we can remove now
the related issue is still active, but current generated code already handles 202 , I think we are good. The new tests I added also worked. So we can close that issue as well

tjprescott · 2016-11-10T13:42:34Z

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

+                                     parent_resource_path or resource_provider_namespace or
+                                     resource_name):
+            raise IncorrectUsageError(
+                "You must specify either 'id' or other individual pieces, but not both")


You might put this in usage statement form, like the error for incorrect use of --ids does. So something like (--id ID | --resource-group RG --name NAME --provider ...)

tjprescott · 2016-11-10T13:46:53Z

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

+register_cli_argument('resource', 'resource_provider_namespace', options_list=('--namespace',), completer=get_providers_completion_list,
+                      help="resource namespaces like 'Microsoft.Network'. You can also specify it through --resource-type parameter")
+register_cli_argument('resource', 'resource_type', help="types such as 'virtualNetworks', or optional include namespace such as 'Microsoft.Network/virtualNetworks'")
+register_cli_argument('resource', 'parent_resource_path', required=False, options_list=('--parent',),


I think we should add these parameters to the same arg_group as is automatically done with the --ids parameter. This would be a more consistent experience even if we don't support the automatic looping mechanic of --ids.

tjprescott · 2016-11-10T20:18:24Z

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

@@ -398,7 +397,7 @@ def __init__(self, resource_group_name=None, resource_provider_namespace=None,
                                     parent_resource_path or resource_provider_namespace or
                                     resource_name):
            raise IncorrectUsageError(
-                "You must specify either 'id' or other individual pieces, but not both")
+                "(--id ID | --resource-group RG --name NAME --namespace NAMESPACE --resource-type TYPE -n NAME)") #pylint: disable=line-too-long


I think the usage string would be (--id ID | --resource-group RG --name NAME [--namespace NAMESPACE] --resource-type TYPE) since namespace is optional and you don't need --name NAME and -n NAME.

vivsriaus

Thanks for making this in!

vivsriaus · 2016-11-11T00:00:19Z

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

+    type: command
+    short-summary: list resource
+    examples:
+        - name: list all resource in a region


I know this is not related to resource Id change, but "list all resource in a region" ? Why? Does it list all resources in a subscription?

vivsriaus · 2016-11-11T00:02:08Z

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

          text: >
-            az resource show -g mygroup -n mywebapp --resource-type "Microsoft.web/sites"
+            az resource show -g myGroup --namespace Microsoft.Network --parent applicationGateways/ag1/urlPathMaps/map1 --resource-type pathRules -n rule1  


We want to move away from this "parent" parameter - only makes it harder for users to identify a resource.

user can use --id, we can't hide the --parent

vivsriaus · 2016-11-11T00:04:03Z

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

+
+helps['resource tag'] = """
+    type: command
+    short-summary: tag a resource. Reference the examples for help with arguments.


I would have expected "resource tag create" to actually add a tag on a resource.

please open an issue, imo, tag is good enough, unless we will have list, 'delete', etc

Yes, we should definitely have a way to delete a tag (unless removal of tag by using update command with nulled out tag properties is already supported)

vivsriaus · 2016-11-11T00:04:21Z

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

+
+helps['resource update'] = """
+    type: command
+    short-summary: update a resource


No help for this?

This is so called generic update, help for common flags is already provided

vivsriaus · 2016-11-11T00:06:37Z

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

+                      help="Provider namespace (Ex: 'Microsoft.Provider')")
+register_cli_argument('resource', 'resource_type',
+                      completer=get_resource_types_completion_list,
+                      help="The resource type (Ex: 'resC'). Can also accept namespace/type format (Ex: 'Microsoft.Provider/resC')")


Why would a resource type also accept provider namespace, like Microsoft.Provider/resc format? Does that mean the provider namespace parameter above is optional?

Correct, the namespace is optional. We saw vm show, vnet show etc, all return namespace/type, so we decided to support it then people can pipe output

vivsriaus · 2016-11-11T00:11:41Z

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

@@ -409,3 +389,124 @@ def get_policy_assignment_completion_list(prefix, **kwargs):#pylint: disable=unu
    result = policy_client.policy_assignments.list()
    return [i.name for i in result]

+class _ResourceUtils(object): #pylint: disable=too-many-instance-attributes
+    def __init__(self, resource_group_name=None, resource_provider_namespace=None,


Don't we validate the provided --id is in the correct format (/subscriptions/subId/resourceGroups/rgName/providers/Microsoft.Compute/vm) before making the request?

By default we don't want to validate input for API which is not LRO and server error is clear. But for this command, we do validation when api-version is missing, which we expect would be the majority case

vivsriaus · 2016-11-11T00:16:26Z

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

+            raise IncorrectUsageError('Resource type {} not found.'
+                                      .format(resource_type_str))
+        if len(rt) == 1 and rt[0].api_versions:
+            npv = [v for v in rt[0].api_versions if 'preview' not in v.lower()]


Just curious - what does the var npv stand for? It isn't obvious to me.

vivsriaus · 2016-11-11T00:17:07Z

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

+            raise IncorrectUsageError('Resource type {} not found.'
+                                      .format(resource_type_str))
+        if len(rt) == 1 and rt[0].api_versions:
+            npv = [v for v in rt[0].api_versions if 'preview' not in v.lower()]


So, we ignore the preview api versions here?

correct. let us know we should use

vivsriaus · 2016-11-11T00:18:05Z

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

+            return npv[0] if npv else rt[0].api_versions[0]
+        else:
+            raise IncorrectUsageError(
+                'API version is required and could not be resolved for resource {}'


I'm not sure if this is even possible. All resource types WILL have an api-version

this is mostly for when the type providerd by users are invalid

vivsriaus · 2016-11-11T00:21:13Z

@yugangw-msft Thanks for making the changes for us. Do all ARM commands work for tenant level (no subscription) resources as well?

yugangw-msft · 2016-11-11T00:34:00Z

Do all ARM commands work for tenant level (no subscription) resources as well?

@vivsriaus I don't think so. Does xplat work for that? Please provide more information

vivsriaus · 2016-11-11T00:39:02Z

@yugangw-msft xplat doesn't, but the generic resource cmdlets in powershell do (New-Resource, Get-Resource, Set-Resource, Remove-Resource). For example, to get a tenant level powerapps resource:

Get-AzureRmResource -ResourceId providers/Microsoft.PowerApps/apps/69fb28f7-9bb3-2766-2b0e-b9449f10fa3c -ApiVersion 2015-02-01-preview

yugangw-msft · 2016-11-11T04:40:24Z

@vivsriaus, good to know. Since only limited resource types can be tenant level, including Intune and PowerApps, we don't need to do it right now.

azurecla added the cla-not-required label Nov 9, 2016

yugangw-msft force-pushed the resid branch 4 times, most recently from 5f61e47 to aaaea9b Compare November 9, 2016 23:45

resource: support show/delete/tag on id

9cf241c

yugangw-msft force-pushed the resid branch from aaaea9b to 755bc79 Compare November 10, 2016 04:25

yugangw-msft changed the title ~~[do not merge]resource: support resource id for show/delete/tag~~ resource: support resource id for show/delete/tag Nov 10, 2016

fix test recoding

7ed7868

yugangw-msft force-pushed the resid branch from 755bc79 to 7ed7868 Compare November 10, 2016 05:59

tjprescott suggested changes Nov 10, 2016

View reviewed changes

yugangw-msft added 2 commits November 10, 2016 11:21

address review feedback

22bec3b

improve helps per review feedback

85e03e9

tjprescott approved these changes Nov 10, 2016

View reviewed changes

yugangw-msft merged commit a71319f into Azure:master Nov 10, 2016

vivsriaus reviewed Nov 11, 2016

View reviewed changes

yugangw-msft mentioned this pull request Nov 11, 2016

resource: support tenant bound resources #1296

Closed

yugangw-msft deleted the resid branch November 12, 2016 18:34

haroldrandom added the cla-not-required label Oct 25, 2019

resource: support resource id for show/delete/tag #1280

resource: support resource id for show/delete/tag #1280

Conversation

yugangw-msft commented Nov 9, 2016

yugangw-msft commented Nov 10, 2016

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft Nov 10, 2016 • edited Loading

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

vivsriaus 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

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

yugangw-msft Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

vivsriaus commented Nov 11, 2016

yugangw-msft commented Nov 11, 2016

vivsriaus commented Nov 11, 2016

yugangw-msft commented Nov 11, 2016

yugangw-msft Nov 10, 2016 •

edited

Loading

yugangw-msft Nov 11, 2016 •

edited

Loading