{Resource} Remove direct call to msrestazure
#29959
Conversation
️✔️AzureCLI-FullTest
|
|
Hi @Jing-song, |
️✔️AzureCLI-BreakingChangeTest
|
|
Resource |
microsoft-github-policy-service
bot
requested review from
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
the
ARM
|
Could you please help migrate the usage of |
Jing-song
requested review from
bebound,
calvinhzy,
evelyn-ys,
jiasli,
kairu-ms,
necusjz and
wangzelin007
as code owners
Done |
| 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) | ||
| 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"})) |
There was a problem hiding this comment.
Could we still obtain the long running status after the migration?
There was a problem hiding this comment.
Yes, it looks the same as before.
There was a problem hiding this comment.
I have re-recorded the test that included the invoice-action command.
There was a problem hiding this comment.
Why is this necessary? My understanding is that removing calls to msrestazure won't affect the HTTP requests.
There was a problem hiding this comment.
Due to replacing AzureOperationPoller in msrestazure with LROPoller in azure.core.polling, so I re-recorded the test
There was a problem hiding this comment.
Changing the poller won't affect the HTTP requests, right?
There was a problem hiding this comment.
Old files will fail
There was a problem hiding this comment.
LROPoler added &monitor=true to the URL and had some parameter order changes, so it needs to rerun the live test, this has no impact on the user
| def deserialization_cb(pipeline_response): | ||
| return json.loads(pipeline_response.http_response.text()) | ||
|
|
||
| request = client.post(url, query_parameters, header_parameters, **body_content_kwargs) | ||
| 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"})) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This is consistent with the previous logic, since there are no functions for it in the SDK, we need to construct them 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"})) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is an option to choose to get the final URL, I will not specify it and let it decide for itself.
| def deserialization_cb(pipeline_response): | ||
| return json.loads(pipeline_response.http_response.text()) | ||
|
|
||
| request = client.post(url, query_parameters, header_parameters, **body_content_kwargs) |
There was a problem hiding this comment.
I am surprised that SDK has no equivalent function and we have to construct the POST request by ourselves.
|
Please rebase to latest |
|
@jiasli Could you please help review this PR again? |
6 participants
Related command
az resource invoke-actionDescription
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 featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.