API Profile Support by derekbekoe · Pull Request #2834 · Azure/azure-cli

derekbekoe · 2017-04-11T22:12:28Z

Use published SDKs available on PyPI now
Loading versioned models for network and resources, object model initialization changes for 2015-* profile (Loading versioned models for network and resources (2015 profile support) #2812)
Custom ca certs - Setting Requests_ca_bundle environment variable (Custom ca certs - Setting Requests_ca_bundle environment variable #2813)
changes for 2015-* profile to work against azure stack (changes for 2015-* profile to work against azure stack #2794)
Loading versioned Resources client and versioned models in VM
Updating the supported api-versions for the 2015-example profile
Fixing network, compute sdk load errors for 2015-* profiles (+5 squashed commit)
Support multi-API versioned Storage dataplane SDK (Support multi-API versioned Storage dataplane SDK #2796)
Support multi-versioned mgmt SDK (Support multi-versioned mgmt SDK #2526)
Fix token "management" endpoint is being used in the place of "activeDirectoryResourceId" (Fix token "management" endpoint is being used in the place of "activeDirectoryResourceId" #2410)
Add profile switching params and profile listing command (Add profile switching params and profile listing command #2398)
Use ARM 'resource manager' endpoint if ASM 'management' endpoint not set

Closes #2578
Closes #2579
Closes #2577

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test.

(see Authoring Command Modules)

codecov-io · 2017-04-11T22:23:48Z

Codecov Report

Merging #2834 into master will increase coverage by 0.07%.
The diff coverage is 75.95%.

@@            Coverage Diff             @@
##           master    #2834      +/-   ##
==========================================
+ Coverage   63.11%   63.19%   +0.07%     
==========================================
  Files         464      466       +2     
  Lines       25916    26125     +209     
  Branches     3948     3995      +47     
==========================================
+ Hits        16358    16509     +151     
- Misses       8507     8536      +29     
- Partials     1051     1080      +29

Impacted Files	Coverage Δ
...re-cli-dla/azure/cli/command_modules/dla/custom.py	`50.47% <0%> (ø)`	⬆️
...tdb/azure/cli/command_modules/documentdb/custom.py	`10.63% <0%> (ø)`	⬆️
...gure/azure/cli/command_modules/configure/custom.py	`15.31% <0%> (ø)`	⬆️
...-cli-iot/azure/cli/command_modules/iot/_factory.py	`100% <100%> (ø)`	⬆️
...azure-cli-core/azure/cli/core/profiles/__init__.py	`100% <100%> (ø)`
...re-cli-sql/azure/cli/command_modules/sql/custom.py	`86.09% <100%> (ø)`	⬆️
...twork/azure/cli/command_modules/network/_params.py	`92.51% <100%> (ø)`	⬆️
...azure-cli-core/azure/cli/core/commands/__init__.py	`81.81% <100%> (+0.32%)`	⬆️
...ure-cli-core/azure/cli/core/commands/validators.py	`97.67% <100%> (ø)`	⬆️
...i-cloud/azure/cli/command_modules/cloud/_params.py	`93.33% <100%> (+1.02%)`	⬆️
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c53a3b...7a3516b. Read the comment docs.

tjprescott

Main concerns:

The api conditional check needs to be improved.
I don't understand when it is appropriate to use get_sdk_attr and get_versioned_models. They seem to do the same thing and it seems they are used interchangeably.
How are we testing this?? Are we to expect that once merged, these services will work correctly against Stack and other clouds?

tjprescott · 2017-04-12T18:16:32Z

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


+    def has_endpoint_set(self, endpoint_name):
+        try:
+            getattr(self, endpoint_name)


Can't this just be simplified to

return hasattr(self, endpoint_name)

The behavior for hasattr for Python 2 vs Python 3 is slightly different.
We actually want the Python 2 implementation.
I'll add a comment in the code to clarify.

tjprescott · 2017-04-12T18:17:54Z

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

        self.is_active = is_active

+    def __str__(self):
+        o = {}


Could be simplfied:

o = { 'profile': self.profile, ... }

tjprescott · 2017-04-12T18:20:18Z

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


+def model_choice_list(resource_type, model_name):
+    model = get_versioned_models(resource_type, model_name)
+    return enum_choice_list(model) if model else {}


Should this return [] as the else since it's an choice list?

enum_choice_list returns an empty dict so it made sense to do the same here.
Actually since the usage is **model_choice_list(), it has to be a dict, not a list.

$ python >>> val = {'n': 1} >>> >>> def f(**kwargs): ... print(kwargs) ... >>> f(**val) {'n': 1} >>> >>> l = [] >>> f(**l) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: f() argument after ** must be a mapping, not list >>>

tjprescott · 2017-04-12T18:22:57Z

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

+    try:
+        return getattr(mod, enum_val_name).value
+    except AttributeError:
+        return None


I'm still concerned about trying to debug a mysterious typo with all of these things being strings. If we were to issue a logger.error, how much spam would we get in --debug for this?

Hmm yes could do that.
Does this work?
logger.debug('Unable to get param default %s.%s for %s', enum_name, enum_val_name, resource_type)
In logs, it would look like:
Unable to get param default Kind.storage for ResourceType.MGMT_STORAGE

Or Skipping param default Kind.storage for ResourceType.MGMT_STORAGE

That's fine--my main question is, does this introduce a ton of spam to --debug? I guess it wouldn't if you were mainly developing against latest, in which case, it would be useful.

Currently, it only appears once. Don't expect it to be too much.

tjprescott · 2017-04-12T18:25:19Z

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

+        raise APIVersionException(resource_type, api_profile)
+
+
+def get_client_class(resource_type):


For this change, exposing the minimum public surface area is important. Right now these all show up as public. Can we update the ones that we don't intend to have used by command authors to use the leading _?

All the methods/attributes/properties in this file are public.

IMO there are way too many public methods then. When people ask "how do I support multiple versions", I would have no idea what to tell them other than to ask you.

This whole shared.py file should not be used by anyone else.
azure/cli/core/profiles/__init__.py is what will be used by others and that has the following public methods: get_api_version, get_sdk, get_versioned_sdk_path each of which has their use.

I have renamed shared.py to _shared.py to make this clearer.

tjprescott · 2017-04-12T18:34:40Z

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


 def cloud_storage_account_service_factory(kwargs):
-    from azure.storage import CloudStorageAccount
+    CloudStorageAccount = get_sdk_attr('azure.multiapi.storage#CloudStorageAccount')


How is "get_sdk_attr" different from "get_versioned_models"?? It seems like you can do the same thing with either.

Yes they are similar. Looking at combining them into one.

tjprescott · 2017-04-12T18:35:47Z

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

+TablePayloadFormat = get_sdk_attr('azure.multiapi.storage.table#TablePayloadFormat')
+QueueService = get_sdk_attr('azure.multiapi.storage.queue#QueueService')
+QueuePermissions = get_sdk_attr('azure.multiapi.storage.queue.models#QueuePermissions')
+PublicAccess = get_sdk_attr('azure.multiapi.storage.blob.models#PublicAccess')


Why are you not using the "get_versioned_models" to eliminate all these copy-paste redundancy?

tjprescott · 2017-04-12T18:38:10Z

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

-    register_cli_argument('storage account {}'.format(item), 'encryption', nargs='+', help='Specifies which service(s) to encrypt.', validator=validate_encryption, **enum_choice_list(list(EncryptionServices._attribute_map.keys())))  # pylint: disable=protected-access
+    register_cli_argument('storage account {}'.format(item), 'sku', help='The storage account SKU.', **model_choice_list(ResourceType.MGMT_STORAGE, 'SkuName'))
+    es_model = get_versioned_models(ResourceType.MGMT_STORAGE, 'EncryptionServices')
+    if es_model:


This seems like an implicit way of saying "if API version > blah". I would highly recommend we be specific here and call out API version explicitly.

Also, why are we using "get_versioned_models" instead of "get_sdk_attr"?

tjprescott · 2017-04-12T18:41:06Z

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

+
+if get_api_version(ResourceType.MGMT_STORAGE) in ['2015-06-15']:
+    cli_command(__name__, 'storage account create', custom_path + 'create_storage_account_with_account_type')
+else:


So any API versions lower or greater than this should use the newer method?

There aren't versions lower than 2015-06-15 so it'd be only newer API versions.

tjprescott · 2017-04-12T18:45:03Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/commands.py

-cli_command(__name__, 'image show', mgmt_path.format(op_var, op_class, 'get'), cf_images, exception_handler=empty_on_404)
-cli_command(__name__, 'image delete', mgmt_path.format(op_var, op_class, 'delete'), cf_images)
+
+if get_api_version(ResourceType.MGMT_COMPUTE) in ['2016-04-30-preview']:


What about VM and VMSS create? Managed disks didn't exist before this API version. I can't image those commands would work as is. I don't see different versions of those commands like Storage Account create.

VM and VMSS create don't fully support older API versions.
This would be part of #2272.

yugangw-msft

Left one extra comment. Thanks for the great effort!

yugangw-msft · 2017-04-13T16:36:57Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_template_builder.py

    # Build VMSS
    vmss_properties = {
        'overprovision': overprovision,
-        'singlePlacementGroup': single_placement_group,


We should keep this, but we need to conditionly to add this if the VM's api-version is >= 2016-04-30-preview

done. thanks.

I thought this PR was not intended to make VM/VMSS create "profile compliant"?

I am fine with different options to address it. But since this PR aims to get into master, then we should not just drop the property, as this clearly makes creating large scale-set no longer work.

Oh, yes, definitely. great catch!

- Use published SDKs available on PyPI now - Loading versioned models for network and resources, object model initialization changes for 2015-* profile (#2812) - Custom ca certs - Setting Requests_ca_bundle environment variable (#2813) - changes for 2015-* profile to work against azure stack (#2794) - Loading versioned Resources client and versioned models in VM - Updating the supported api-versions for the 2015-example profile - Fixing network, compute sdk load errors for 2015-* profiles (+5 squashed commit) - Support multi-API versioned Storage dataplane SDK (#2796) - Support multi-versioned mgmt SDK (#2526) - Fix token "management" endpoint is being used in the place of "activeDirectoryResourceId" (#2410) - Add profile switching params and profile listing command (#2398) - Use ARM 'resource manager' endpoint if ASM 'management' endpoint not set

derekbekoe · 2017-04-13T23:42:54Z

@tjprescott Can you take another look?
I've addressed all comments apart from creating convenience methods like "api_version_greater_than" as we need to get clarity on API version ordering.

derekbekoe · 2017-04-13T23:51:22Z

Main concerns:

The api conditional check needs to be improved.

I don't understand when it is appropriate to use get_sdk_attr and get_versioned_models. They seem to do the same thing and it seems they are used interchangeably.

How are we testing this?? Are we to expect that once merged, these services will work correctly against Stack and other clouds?

1 requires an api ordering which is currently not known.
2. Fixed this. There is a single get_sdk function.
3. It's not expected to completely work on Stack or other clouds yet.

Fix pylint and pep8 (+7 squashed commits) Squashed commits: [f1d4d52] Refactor to use joint method [f43785a] Use generic method to get versioned SDK attributes [2100dd2] Add log debug if enum default not found [5767f41] Add single_placement_group back in for vmss [05ca8bc] Revert a test change. It has been fixed in master. [cdf4211] Simplify str override method [281c755] Clarify use of getattr

tjprescott · 2017-04-14T17:43:25Z

I'm still struggling with these issues:

The surface area of the versioning APIs. I see the following:
profiles.__init__ has get_api_verison, get_sdk, get_versioned_sdk_path
profiles._shared has get_api_version, get_client_class, get_versioned_sdk_path, get_versioned_sdk
So if _shared is all internal, okay, cool. What's the difference between get_sdk and get_versioned_sdk_path?
I don't think merging this without a solution for "API version > blah" makes sense. To me this seems like a bare minimum requirement to have maintainable versioning APIs for people to code against.

troydai · 2017-04-14T17:43:48Z

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

-logger.debug("Current active cloud '%s'", CLOUD.name)
-logger.debug(pformat(vars(CLOUD.endpoints)))
-logger.debug(pformat(vars(CLOUD.suffixes)))
+logger.debug(str(CLOUD))


Unless the to string conversion includes a readable explanation of what it is, I would suggest print a more readable message like:

logger.debug('Current cloud is {}'.format(str(CLOUD)))

It looks like this:

{'endpoints': {'active_directory': 'https://login.microsoftonline.com', 'active_directory_graph_resource_id': 'https://graph.windows.net/', 'active_directory_resource_id': 'https://management.core.windows.net/', 'batch_resource_id': None, 'gallery': 'https://gallery.azure.com/', 'management': 'https://management.core.windows.net/', 'resource_manager': 'https://management.azure.com/', 'sql_management': 'https://management.core.windows.net:8443/'}, 'is_active': True, 'name': 'AzureCloud', 'profile': '2016-sample', 'suffixes': {'azure_datalake_analytics_catalog_and_job_endpoint': 'azuredatalakeanalytics.net', 'azure_datalake_store_file_system_endpoint': 'azuredatalakestore.net', 'keyvault_dns': '.vault.azure.net', 'sql_server_hostname': '.database.windows.net', 'storage_endpoint': 'core.windows.net'}}

I've added a message though so at least it's possible to search for the string to find which line of code creates that log message.

…files

troydai

Looks good to me.

troydai · 2017-04-14T17:51:55Z

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

                setattr(c.suffixes, option.replace('suffix_', ''), config.get(section, option))
+        if c.profile is None:
+            # If profile isn't set, use latest
+            c.profile = 'latest'  # pylint: disable=redefined-variable-type


will use setAttr avoid the the pylint error?

yes that works. thanks for the suggestion!

derekbekoe · 2017-04-14T22:15:17Z

@tjprescott
I have removed get_versioned_sdk_path and inlined it to the only place it was used.
Also, see the commit below for the changes to support specifying API version ranges (currently just supports the basic date format as that's all we have right now).
b84f60e

tjprescott

LGTM! Thanks for putting up with me! 😁

derekbekoe · 2017-04-14T23:09:39Z

merged 🎉 🍾

msftclas added the cla-not-required label Apr 11, 2017

derekbekoe requested review from johanste, tjprescott, troydai and yugangw-msft and removed request for tjprescott and yugangw-msft April 11, 2017 22:25

tjprescott suggested changes Apr 12, 2017

View reviewed changes

yugangw-msft reviewed Apr 13, 2017

View reviewed changes

derekbekoe force-pushed the api-profile-support branch from 154f99f to 3797a17 Compare April 13, 2017 23:39

derekbekoe force-pushed the api-profile-support branch from c4842ac to bef3a48 Compare April 14, 2017 00:03

derekbekoe added 3 commits April 13, 2017 17:26

Fix patching the operation versions

6351668

Fix wrong import

84b6cbe

Rename shared to _shared

a671d42

troydai reviewed Apr 14, 2017

View reviewed changes

derekbekoe added 2 commits April 14, 2017 10:48

Add message to cloud debug message

5a9d96c

Remove public method ‘get_versioned_sdk_path’ from azure.cli.core.pro…

7ade9ce

…files

troydai approved these changes Apr 14, 2017

View reviewed changes

derekbekoe added 3 commits April 14, 2017 12:31

Remove unused import

a4c34ae

Use setattr

cd4ed12

Add API version comparisons

b84f60e

tjprescott approved these changes Apr 14, 2017

View reviewed changes

derekbekoe added 2 commits April 14, 2017 15:50

Add method docs

944f27e

Fix failing test

7a3516b

derekbekoe merged commit bed55b1 into master Apr 14, 2017

derekbekoe deleted the api-profile-support branch April 14, 2017 23:08

tjprescott mentioned this pull request Jun 5, 2017

[Network] Live test failures #3386

Closed

2 tasks

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

		raise APIVersionException(resource_type, api_profile)


		def get_client_class(resource_type):

Conversation

derekbekoe commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General Guidelines

Command Guidelines

Uh oh!

codecov-io commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbekoe Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbekoe commented Apr 11, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited

Loading

derekbekoe Apr 12, 2017 •

edited

Loading

derekbekoe commented Apr 13, 2017 •

edited by tjprescott

Loading

troydai Apr 14, 2017 •

edited

Loading