azure-http-specs, add ARM LRO case #1821
Conversation
|
All changed packages have been documented.
|
|
You can try these changes here
|
XiaofeiCao
requested review from
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
|
I don't get this line
Why DELETE got a finalResult? |
I mean get final-state. In our core, we do have logic for POST/DELETE to get final state when poll succeeded: Mark is also mentioning this pattern: https://teams.cloud.microsoft/l/message/19:[email protected]/1729624003961?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&parentMessageId=1728977564024&teamName=Azure%20SDK&channelName=TypeSpec%20Discussion&createdTime=1729624003961 Do you mean it's uncommon for DELETE to do a get final state? Or am I confusing finalResult with finalState? |
Changed description to use final-state instead of finalResult. |
I think for DELETE via AAO, when I would take majority of this AAO/Location case is for POST, where AAO header is for status (and final-state), while Location header is for finalResult. |
Makes sense. Let me update the case be:
|
packages/azure-http-specs/specs/azure/resource-manager/operation-templates/lro.tsp
Outdated
Show resolved
Hide resolved
Yeah, I'm going to cover that, see #1821 (comment) |
packages/azure-http-specs/specs/azure/resource-manager/operation-templates/lro.tsp
Outdated
Show resolved
Hide resolved
packages/azure-http-specs/specs/azure/resource-manager/operation-templates/lro.tsp
Outdated
Show resolved
Hide resolved
Added resource action case. |
| { | ||
| "url": "https://example.org/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Azure.ResourceManager.OperationTemplates/lroResources/lro/files/myfile" | ||
| } | ||
| ``` |
There was a problem hiding this comment.
For POST scenario, I see your comment in PR body that SDK shall poll by AAO and get final result by Location. However, the scenarioDoc still shows way to poll for Location.
There was a problem hiding this comment.
Yes, for AAO+Location, the preferred polling header is AAO. Here I also provided way of polling through Location.
You may choose to poll through either one, since from my understanding, neither of them violates ARM guideline. Do correct me if I'm wrong here.
| } | ||
|
|
||
| model UploadResult { | ||
| url: string; |
There was a problem hiding this comment.
nit, I guess .NET would want to use URL type?
There was a problem hiding this comment.
This is a regular response body property, though sure, let me make it type url.
There was a problem hiding this comment.
updated to use ExportResult
| final-state-via: Azure-AsyncOperation | ||
|
|
||
| Expected verb: PUT | ||
| Expected path: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Azure.ResourceManager.OperationTemplates/lroResources/lro |
There was a problem hiding this comment.
nit, I'd prefer resource name not be "lro". It could be confusing for all the "/lroResources/lro" in path, while none of them really signifies the operation be LRO or not (they are just resource collection and resource name).
Just a nit, you may keep this, if it is just my feeling.
There was a problem hiding this comment.
Updated to make more sense.
| model LroResourceProperties { | ||
| @doc("The description of the resource.") | ||
| description?: string; | ||
| } |
There was a problem hiding this comment.
Do we need to have provisioningState?: ProvisioningState; in properties?
There was a problem hiding this comment.
Yes, we do. In mockapi I returned it, here we should have that too.
| Expected verb: GET | ||
| Expected URL: {endpoint}/subscriptions/00000000-0000-0000-0000-000000000000/locations/eastus/lro_create/aao | ||
|
|
||
| Expected status code: 200 | ||
| Expected response body: | ||
| ```json | ||
| { | ||
| "id": "/subscriptions/00000000-0000-0000-0000-000000000000/locations/eastus/lro_create/aao", | ||
| "name": "aao", | ||
| "status" : "Succeeded", | ||
| "startTime": "2024-11-08T01:41:53.5508583+00:00", | ||
| "endTime": "2024-11-08T01:42:41.5354192+00:00", | ||
| "properties": { | ||
| "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Azure.ResourceManager.OperationTemplates/lroResources/lro", | ||
| "name": "lro", | ||
| "type": "Azure.ResourceManager.Resources/lroResources", | ||
| "location": "eastus", | ||
| "properties": { | ||
| "description": "valid", | ||
| "provisioningState": "Succeeded" | ||
| }, | ||
| "systemData": { | ||
| "createdBy": "AzureSDK", | ||
| "createdByType": "User", | ||
| "createdAt": <any date>, | ||
| "lastModifiedBy": "AzureSDK", | ||
| "lastModifiedAt": <any date>, | ||
| "lastModifiedByType": "User", | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
What's behavior of real ARM lro in RP?
I kind of think this should be same AAO status response, not resource response (and the final GET to original resource URL not optional for MGMT) -- as the endpoint is the AAO endpoint, it should return consistent body schema. Though I could be wrong.
{
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/locations/eastus/lro_create/aao",
"name": "aao",
"startTime": <date>,
"status" : "Succeeded"
}
There was a problem hiding this comment.
Yes, it's AAO response. I added property properties mentioned in the guideline, though I haven't seeing that in any real service.
OK, since final GET in mgmt is required, I'll remove this property.
| #suppress "deprecated" "legacy" | ||
| #suppress "@azure-tools/typespec-azure-resource-manager/lro-location-header" "legacy" | ||
| @scenario | ||
| @scenarioDoc(""" |
There was a problem hiding this comment.
curious, what's typespec-azure-resourcemanager's non-legacy DELETE? Is it AAO or Location?
There was a problem hiding this comment.
Emmm, it's Location. Let me remove the unnecessary header.
| Expected path: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Azure.ResourceManager.OperationTemplates/orders/order1 | ||
| Expected query parameter: api-version=2023-12-01-preview | ||
| Expected response status code: 202 | ||
| Expected response header: Location={endpoint}/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/lro_delete/location |
There was a problem hiding this comment.
i believe all polling url is similar patten. refer this. it's better to mock more real.
|
|
||
| Location first poll. | ||
| Expected verb: GET | ||
| Expected URL: {endpoint}/subscriptions/00000000-0000-0000-0000-000000000000/locations/eastus/lro_delete/location |
There was a problem hiding this comment.
seems different from the response?
| Expected status code: 202 | ||
| Expected no response body |
There was a problem hiding this comment.
the polling will always return 202? from recording, it seems always 200. but from guideline, it seems right.
| } | ||
| ``` | ||
| """) | ||
| @useFinalStateVia("location") |
There was a problem hiding this comment.
for normal situation, we don't add such decorator.
| "lastModifiedByType": "User", | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Need an end to the literal format block here (```)
Context
Content