Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ARM] Migrate resource to track2 SDK #17783

Merged
merged 4 commits into from
May 12, 2021
Merged

Conversation

zhoxing-ms
Copy link
Contributor

@zhoxing-ms zhoxing-ms commented Apr 20, 2021

Description

This PR upgrades the Python SDK version of azure-mgmt-resource from 12.1.0 to 16.1.0

Here are the main changes for resource migration to track2:

  1. LRO operations are renamed to begin_xxx_xxx. For example in track1, the name is create_or_update; while in track2, the name is begin_create_or_update.
  2. When api-version is less than 2019-10-01, the parameters of all deployment operations are changed from DeploymentProperties to Deployment, which is consistent with the SDK after version 2019-10-01.
  3. The objects of input parameters for some methods are further encapsulated:
  • begin_what_if(): DeploymentWhatIfProperties --> DeploymentWhatIf
  • begin_what_if_at_subscription_scope()/...: DeploymentWhatIfProperties --> ScopedDeploymentWhatIf
  • begin_export_template(): flattened parameters --> ExportTemplateRequest
  • begin_move_resources(): flattened parameters --> ResourcesMoveInfo
  • links_client.create_or_update(): ResourceLinkProperties --> ResourceLink
  • tags.create_or_update_at_scope(): flattened parameters --> TagsResource
  • tags.update_at_scope(): flattened parameters --> TagsPatchResource
  1. In method get() of ResourcesOperations, some parameters are passed in through **kwargs instead
  2. CloudError and HttpOperationError are replaced by HttpResponseError
  3. In track 2, the logic corresponding to the class name of Codegen generating operations changes, so DeploymentOperations becomes DeploymentOperationsOperations
    For more details, please refer to this issue: remove double "operations" in operation group names autorest.python#910 (comment)
  4. config in management client becomes a private variable _config.
  5. In method policy_definitions.create_or_update_at_management_group(), the positions of parameters management_groupand parameters are reversed.
  6. generate_client_request_id in ResourceManagementClientConfiguration no longer needs, SDK automatically add x-ms-request-id now through the RequestIdPolicy
  7. The accept_language is no longer supported in ResourceManagementClientConfiguration, so the fixed value en-US in track 2 is hard coded.
  8. The pipeline and policy in management client migrated from msrest to azure-core. And when doing ARM deployment, the pipeline needs to add custom policy JsonCTemplatePolicy() to support JSON templates.
  9. get_by_id() and get() of ResourcesOperations no longer support parameter raw, but passes in a callback method cls instead. So the custom callback function add_response_body() is used to realize the logic of include_response_body.
  10. The resource action lacks generic POST operation in Swagger, so for the time being, AzureOperationPoller of msrestis still used for low-cost migration. And the request way is changed to pipeline.run in track 2.
  11. Before version 2019-08-01, get_by_id() and get() of ResourcesOperations lacked parameter api-version. The problem was solved by modifying the Swagger to regenerate a new Python SDK. PR link: Swagger PR
  12. Fix some live tests due to the changes in track2.

Known commands that are not covered by test

  1. appservice domain create
  2. appservice ase create

Migration for extension
PR link: Azure/azure-cli-extensions#3355

Live Test
result link

TODO

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


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

@yonzhan
Copy link
Collaborator

yonzhan commented Apr 20, 2021

ARM

@yonzhan yonzhan added this to the S186 milestone Apr 20, 2021
@zhoxing-ms zhoxing-ms force-pushed the resource_track2 branch 3 times, most recently from fe95df2 to 782fff3 Compare April 21, 2021 06:27
@zhoxing-ms zhoxing-ms marked this pull request as ready for review April 21, 2021 14:57
],
sender=PipelineRequestsHTTPSender(RequestsHTTPSender(smc.config))
from azure.core.pipeline import Pipeline
smc._client._pipeline._impl_policies.append(JsonCTemplatePolicy())
Copy link
Member

@jiasli jiasli Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using per_call_policies introduced by Azure/azure-sdk-for-python#17340.

It requires azure-core 1.13.0 which is bumped by #17671.

@@ -3188,34 +3195,35 @@ def invoke_action(self, action, request_body):
# Construct headers
header_parameters = {}
header_parameters['Content-Type'] = 'application/json; charset=utf-8'
if self.rcf.resources.config.generate_client_request_id:
header_parameters['x-ms-client-request-id'] = str(uuid.uuid4())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_prepare_client_kwargs_track2 from core already configures x-ms-client-request-id.

# Prepare x-ms-client-request-id header, used by RequestIdPolicy
if 'x-ms-client-request-id' in cli_ctx.data['headers']:
client_kwargs['request_id'] = cli_ctx.data['headers']['x-ms-client-request-id']

@jiasli
Copy link
Member

jiasli commented Apr 22, 2021

Migrate for extension

I would suggest we provide a method for users to revert back to old versions of Azure CLI because these are not small changes and may break extensions, especially those not in azure-cli-extensions (#13653).

@jiasli
Copy link
Member

jiasli commented Apr 22, 2021

Please kindly put this PR on hold until #17671 and #17812 are merged so that they can be tested together.

Update: ✔ #17671 and #17812 are merged. Please merge from dev.

Comment on lines +248 to +249
Deployment = cmd.get_models('Deployment', resource_type=ResourceType.MGMT_RESOURCE_RESOURCES)
deployment = Deployment(properties=properties)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get the Deployment model from SDK which api-version less than 2019-10-01

Copy link
Contributor Author

@zhoxing-ms zhoxing-ms Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because in track 2, the SDK before version 2019-10-01 has added this model
Related code: code link

Comment on lines +3337 to +3338
Deployment = cmd.get_models('Deployment', resource_type=ResourceType.MGMT_RESOURCE_RESOURCES)
deployment = Deployment(properties=properties)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern as above.

@yonzhan yonzhan modified the milestones: S186, S187 May 1, 2021
Copy link
Member

@qwordy qwordy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve vm part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants