Support custom resource types being added to API_PROFILES #6173

derekbekoe · 2018-04-20T21:19:22Z

Only list through RTs in the current profile instead of all (no need to go through all)

Marked as DNM for now to get feedback and also, it includes a change to storage that we wouldn't want to actually merge.

promptws · 2018-04-20T21:19:23Z

View a preview at https://prompt.ws/r/Azure/azure-cli/6173
This is an experimental preview for @microsoft users.

williexu

This will be helpful for extensions. Just comments about how this will be used.

williexu · 2018-04-20T21:45:27Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/__init__.py

        super(StorageCommandsLoader, self).__init__(cli_ctx=cli_ctx,
                                                    resource_type=ResourceType.DATA_STORAGE,
+                                                    custom_resource_types=custom_resource_types,


Kind of strange adding it here. The other kwargs are more sort of defaults that the commands from this loader will use.
The custom_resource_types kwargs is used to inject new resourcetypes to the CLI.
Calling a function from core might make more sense.

Moved to calling a function.

williexu · 2018-04-20T21:46:58Z

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

+            from azure.cli.core.profiles import API_PROFILES
+            for profile, v in custom_resource_types.items():
+                if profile in API_PROFILES:
+                    API_PROFILES[profile].update(v)


Instead of here, how about adding this update as a callable function in azure.cli.core.profiles?

I've gone with that approach.

tjprescott · 2018-04-23T17:45:21Z

src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/__init__.py

 class StorageCommandsLoader(AzCommandsLoader):
    def __init__(self, cli_ctx=None):
        from azure.cli.core.commands import CliCommandType

        storage_custom = CliCommandType(operations_tmpl='azure.cli.command_modules.storage.custom#{}')
+        custom_resource_types = {'latest': {CUSTOM_MGMT_STORAGE: '2015-06-15'}}


This structure isn't really intuitive. It should be validated in the init. Otherwise an error will likely result in a stack trace.

This will be resolved with Willie's suggestion.

derekbekoe · 2018-04-24T22:55:52Z

Addressed feedback, please take another look.

Usage would be something like this:

class StorageCommandsLoader(AzCommandsLoader):
     def __init__(self, cli_ctx=None):
         CUSTOM_MGMT_STORAGE = CustomResourceType('azure.mgmt.storage', 'StorageManagementClient')
        augment_api_profiles('latest', CUSTOM_MGMT_STORAGE, '2015-06-15')

tjprescott

Some comments on the augment_api_profile syntax.

tjprescott · 2018-04-24T23:32:10Z

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

@@ -89,3 +89,15 @@ def get_sdk(cli_ctx, resource_type, *attr_args, **kwargs):
    'latest': AZURE_API_PROFILES['latest'],
    '2017-03-09-profile': AZURE_API_PROFILES['2017-03-09-profile']
 }
+
+
+def augment_api_profiles(profile_name, resource_type, api_version):


So if I wanted to add a resource type to multiple profiles I would have:

augment_api_profiles('latest', MY_RESOURCE_TYPE, '2016-05-01') augment_api_profiles('2017-01-01-profile', MY_RESOURCE_TYPE, '2016-05-01') ...

A more concise syntax to consider:

register_resource_type(MY_RESOURCE_TYPE, { 'latest': '2016-05-01', '2017-01-01-profile': '2016-05-01' })

Essentially you specify the resource type once and then a dictionary of profile: version pairs.

Requiring a dictionary argument seems confusing.

I agree with @williexu, the dict format seems confusing and that was an original comment in my first implementation that allowed users to pass in a dict.

I could potentially change the method name from augment_api_profiles to register_resource_type if that's better for everyone.

yes to register_resource_type

I didn't see anything wrong with the dictionary implementation in the original. I don't like that I would have to repeat so much code repeatedly calling "augment_api_profiles" (or "register_resource_type") once for each resource type and each profile. I'd be curious what @troydai thinks here.

Something that looks closest to the API profile structure seems most logical to me (I have a feeling that's probably what your original implementation looked like--but I didn't have problems with that).

register_resource_types({ 'latest': { MY_RT_1: '2018-01-01', MY_RT_2: '2018-01-01' } '2018-01-01-profile': { MY_RT_1: '2018-01-01', MY_RT_2: '2018-01-01' } })

It's essentially just a profile dictionary merge. I don't think that's necessarily confusing--just would just need to validate it.

@tjprescott why not:

for profile in 'latest', '2018-...': for rt in RT_1, RT_2: register_resource_type(profile, rt, version)

tjprescott · 2018-04-24T23:33:18Z

src/azure-cli-core/azure/cli/core/profiles/_shared.py

@@ -24,6 +24,12 @@ def __str__(self):
 PROFILE_TYPE = object()


+class CustomResourceType(object):  # pylint: disable=too-few-public-methods


Also, what if you have to add an endpoint type in addition to or instead of a resource type? How would that be supported?

That would be part of #4786 which this does not address.

Would it not make sense to implement these both at the same time?

williexu

Looks good

tjprescott

Won't block this, but I think we should tackle both resource types and endpoints at the same time so that the mechanisms are similar, and we should consider a mechanism that reduces the amount of duplication. Something that favors the natural profile dict structure makes sense to me.

derekbekoe · 2018-04-25T00:06:15Z

While Profiles and Endpoints for clouds are related, they are intentionally separated in the source code because you can use the same profile with multiple clouds and there's no relation between the two in terms of Azure.

That's why the cloud and profile code lives separately:
https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/cloud.py
https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/profiles/__init__.py

I can take a look at adding support for that next though...

troydai · 2018-04-25T16:16:37Z

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

        from azure.cli.core.profiles._shared import get_versioned_sdk_path

-        for rt in ResourceType:
+        resource_types_in_profile = AZURE_API_PROFILES[self.cli_ctx.cloud.profile].keys()


if AZURE_API_PROFILE is a map, you don't need the .keys() here. Iterate a map is iterating on its keys.

>>> m={'a':1,'b':2} >>> for i in m: ... print(i) ... a b

Any chance a KeyError throws here?

Removed use of .keys()

…mmits) Squashed commits: [9e2b19ca3] Fix style [70e140f49] Undo the changes used to test [40e5a10ba] Address feedback and add tests (+1 squashed commit) Squashed commits: [3f36f10ca] Support custom resource types being added to API_PROFILES - Only list through RTs in the current profile instead of all (no need to go through all)

derekbekoe added the do-not-merge label Apr 20, 2018

derekbekoe requested review from troydai and williexu April 20, 2018 21:19

williexu reviewed Apr 20, 2018

View reviewed changes

tjprescott reviewed Apr 23, 2018

View reviewed changes

derekbekoe force-pushed the extended-profiles branch 3 times, most recently from 3f36f10 to 40e5a10 Compare April 24, 2018 22:53

derekbekoe added do-not-merge and removed do-not-merge labels Apr 24, 2018

derekbekoe added this to the Sprint 36 (Build) milestone Apr 24, 2018

tjprescott reviewed Apr 24, 2018

View reviewed changes

tjprescott added the Infrastructure label Apr 24, 2018

williexu approved these changes Apr 24, 2018

View reviewed changes

tjprescott reviewed Apr 24, 2018

View reviewed changes

troydai approved these changes Apr 25, 2018

View reviewed changes

derekbekoe removed the do-not-merge label Apr 25, 2018

derekbekoe force-pushed the extended-profiles branch from a8366bd to 5648f00 Compare April 25, 2018 18:22

derekbekoe added 2 commits April 25, 2018 14:16

No need to use keys

6b6a9ac

derekbekoe force-pushed the extended-profiles branch from 5648f00 to 6b6a9ac Compare April 25, 2018 21:18

derekbekoe merged commit 7402fb2 into Azure:dev Apr 25, 2018

derekbekoe deleted the extended-profiles branch April 25, 2018 21:57

haroldrandom added the Infrastructure label Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom resource types being added to API_PROFILES #6173

Support custom resource types being added to API_PROFILES #6173

derekbekoe commented Apr 20, 2018

promptws commented Apr 20, 2018

williexu left a comment

williexu Apr 20, 2018

derekbekoe Apr 24, 2018

williexu Apr 20, 2018

derekbekoe Apr 24, 2018

tjprescott Apr 23, 2018

derekbekoe Apr 24, 2018

derekbekoe commented Apr 24, 2018

tjprescott left a comment

tjprescott Apr 24, 2018

williexu Apr 24, 2018

derekbekoe Apr 24, 2018

williexu Apr 24, 2018

tjprescott Apr 24, 2018

williexu Apr 25, 2018

tjprescott Apr 24, 2018

derekbekoe Apr 24, 2018

tjprescott Apr 24, 2018

williexu left a comment

tjprescott left a comment

derekbekoe commented Apr 25, 2018

troydai Apr 25, 2018

troydai Apr 25, 2018

derekbekoe Apr 25, 2018

		@@ -24,6 +24,12 @@ def __str__(self):
		PROFILE_TYPE = object()


		class CustomResourceType(object): # pylint: disable=too-few-public-methods

Support custom resource types being added to API_PROFILES #6173

Support custom resource types being added to API_PROFILES #6173

Conversation

derekbekoe commented Apr 20, 2018

promptws commented Apr 20, 2018

williexu 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

derekbekoe commented Apr 24, 2018

tjprescott 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

williexu left a comment

Choose a reason for hiding this comment

tjprescott left a comment

Choose a reason for hiding this comment

derekbekoe commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment