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

{Resource} Remove direct call to msrestazure #29959

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/azure-cli-core/azure/cli/core/commands/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1214,7 +1214,7 @@ def _is_paged(obj):

def _is_poller(obj):
# Since loading msrest is expensive, we avoid it until we have to
if obj.__class__.__name__ in ['AzureOperationPoller', 'LROPoller', 'AAZLROPoller']:
if obj.__class__.__name__ in ['LROPoller', 'AAZLROPoller']:
return isinstance(obj, poller_classes())
return False

Expand Down
3 changes: 1 addition & 2 deletions src/azure-cli-core/azure/cli/core/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -676,11 +676,10 @@ def should_disable_connection_verify():


def poller_classes():
from msrestazure.azure_operation import AzureOperationPoller
from msrest.polling.poller import LROPoller
from azure.core.polling import LROPoller as AzureCoreLROPoller
from azure.cli.core.aaz._poller import AAZLROPoller
return (AzureOperationPoller, LROPoller, AzureCoreLROPoller, AAZLROPoller)
return (LROPoller, AzureCoreLROPoller, AAZLROPoller)


def augment_no_wait_handler_args(no_wait_enabled, handler, handler_args):
Expand Down
5 changes: 1 addition & 4 deletions src/azure-cli-testsdk/azure/cli/testsdk/patches.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,7 @@ def _shortcut_long_run_operation(*args, **kwargs): # pylint: disable=unused-arg
return

mock_in_unit_test(unit_test,
'msrestazure.azure_operation.AzureOperationPoller._delay',
_shortcut_long_run_operation)
mock_in_unit_test(unit_test,
'msrestazure.polling.arm_polling.ARMPolling._delay',
'azure.mgmt.core.polling.arm_polling.ARMPolling._delay',
_shortcut_long_run_operation)
mock_in_unit_test(unit_test,
'azure.cli.core.commands.LongRunningOperation._delay',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,6 @@ def setUp(self):

def tearDown(self):
os.environ = self.original_env
# Autorest.Python 2.x
assert not [t for t in threading.enumerate() if t.name.startswith("AzureOperationPoller")], \
"You need to call 'result' or 'wait' on all AzureOperationPoller you have created"
# Autorest.Python 3.x
assert not [t for t in threading.enumerate() if t.name.startswith("LROPoller")], \
"You need to call 'result' or 'wait' on all LROPoller you have created"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def _shortcut_long_run_operation(*args, **kwargs): # pylint: disable=unused-arg
return

mock_in_unit_test(unit_test,
'msrestazure.azure_operation.AzureOperationPoller._delay',
'azure.mgmt.core.polling.arm_polling.ARMPolling._delay',
_shortcut_long_run_operation)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def internal_validate_lock_parameters(namespace, resource_group, resource_provid
parent_resource_path, resource_type, resource_name):
if resource_group is None:
if resource_name is not None:
from msrestazure.tools import parse_resource_id, is_valid_resource_id
from azure.mgmt.core.tools import parse_resource_id, is_valid_resource_id
evelyn-ys marked this conversation as resolved.
Show resolved Hide resolved
if not is_valid_resource_id(resource_name):
raise CLIError('--resource is not a valid resource ID. '
'--resource as a resource name is ignored if --resource-group is not given.')
Expand Down
40 changes: 14 additions & 26 deletions src/azure-cli/azure/cli/command_modules/resource/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from urllib.request import urlopen
from urllib.parse import urlparse, unquote

from msrestazure.tools import is_valid_resource_id, parse_resource_id
from azure.mgmt.core.tools import is_valid_resource_id, parse_resource_id

from azure.mgmt.resource.resources.models import GenericResource, DeploymentMode

Expand Down Expand Up @@ -66,7 +66,7 @@


def _build_resource_id(**kwargs):
from msrestazure.tools import resource_id as resource_id_from_dict
from azure.mgmt.core.tools import resource_id as resource_id_from_dict
try:
return resource_id_from_dict(**kwargs)
except KeyError:
Expand Down Expand Up @@ -3257,7 +3257,7 @@ def create_policy_assignment(cmd, policy=None, policy_set_definition=None,


def _get_resource_id(cli_ctx, val, resource_group, resource_type, resource_namespace):
from msrestazure.tools import resource_id
from azure.mgmt.core.tools import resource_id
if is_valid_resource_id(val):
return val

Expand Down Expand Up @@ -4522,7 +4522,9 @@ def invoke_action(self, action, request_body):
"""
Formats Url if none provided and sends the POST request with the url and request-body.
"""
from msrestazure.azure_operation import AzureOperationPoller

from azure.core.polling import LROPoller
from azure.mgmt.core.polling.arm_polling import ARMPolling

query_parameters = {}
serialize = self.rcf.resources._serialize # pylint: disable=protected-access
Expand Down Expand Up @@ -4564,28 +4566,14 @@ def invoke_action(self, action, request_body):
body_content_kwargs = {}
body_content_kwargs['content'] = json.loads(request_body) if request_body else None

# Construct and send request
def long_running_send():
request = client.post(url, query_parameters, header_parameters, **body_content_kwargs)
pipeline_response = client._pipeline.run(request, stream=False)
return pipeline_response.http_response.internal_response

def get_long_running_status(status_link, headers=None):
request = client.get(status_link, query_parameters, header_parameters)
if headers:
request.headers.update(headers)
pipeline_response = client._pipeline.run(request, stream=False)
return pipeline_response.http_response.internal_response

def get_long_running_output(response):
from azure.core.exceptions import HttpResponseError
if response.status_code not in [200, 202, 204]:
exp = HttpResponseError(response)
exp.request_id = response.headers.get('x-ms-request-id')
raise exp
return response.text

return AzureOperationPoller(long_running_send, get_long_running_output, get_long_running_status)
def deserialization_cb(pipeline_response):
return json.loads(pipeline_response.http_response.text())

request = client.post(url, query_parameters, header_parameters, **body_content_kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised that SDK has no equivalent function and we have to construct the POST request by ourselves.

pipeline_response = client._pipeline.run(request, stream=False)

return LROPoller(client=client, initial_response=pipeline_response, deserialization_callback=deserialization_cb,
polling_method=ARMPolling(lro_options={"final-state-via": "azure-async-operation"}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we still obtain the long running status after the migration?

Copy link
Contributor Author

@Jing-song Jing-song Sep 27, 2024

Choose a reason for hiding this comment

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

Yes, it looks the same as before.
image
image
image

Copy link
Member

@jiasli jiasli Sep 29, 2024

Choose a reason for hiding this comment

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

I am not an expert on this part, but this process seems to be pretty generic. Do we have to re-define the whole LRO process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is consistent with the previous logic, since there are no functions for it in the SDK, we need to construct them ourselves.

Copy link
Member

Choose a reason for hiding this comment

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

I understand the name azure-async-operation is from SDK (https://github.com/Azure/azure-sdk-for-python/blob/54b2d3701a530628d52936193c3a5f564fbbee73/sdk/core/azure-mgmt-core/azure/mgmt/core/polling/arm_polling.py#L69), but it is super confusing as the header name in the HTTP response is Azure-AsyncOperation as documented by https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/async-operations. azure-async-operation is never used in any place.

Copy link
Contributor Author

@Jing-song Jing-song Sep 29, 2024

Choose a reason for hiding this comment

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

It is an option to choose to get the final URL, I will not specify it and let it decide for itself.


@staticmethod
def resolve_api_version(rcf, resource_provider_namespace, parent_resource_path, resource_type,
Expand Down
Loading