Azure / autorest.powershell

AutoRest PowerShell Generator
MIT License
111 stars 76 forks source link

Add support for x-ms-long-running-operation-options.final-state-schema #1196

Open mikekistler opened 1 year ago

mikekistler commented 1 year ago

Autorest has added a new field to the x-ms-long-running-operation-options specification extension, final-state-schema.

https://github.com/Azure/autorest/tree/main/docs/extensions#x-ms-long-running-operation-options

final-state-schema can be used to point to the definition of the response schema returned from the final-state-via. Previously this schema was specified on the 200 response for the operation, but this was problematic because the operation may not actually return 200. final-state-schema allows the API author to avoid "fibbing" about the 200 response just to make autorest work.

autorest.powershell should also add support for final-state-schema.

dolauli commented 1 year ago

@mikekistler, Based on your recommendation, I attempted to generate a module using core@3.9.6 and modelerfour@4.26.2. However, it appears that final-state-schema is not fully supported in modelerfour 4.26.2.

autorest --powershell --use:@autorest/powershell@4.x --use:@autorest/modelerfour@4.26.2 --version:3.9.6 --input-file:https://github.com/dolauli/share/blob/main/swaggers/pollingPaging.json --interactive

Here is is the serialized modelerfour file.

mikekistler commented 1 year ago

@dolauli I have tested this myself and I think it is working correctly. Let me tell you how I tested to see if you agree.

First I had to find a suitable API definition -- one that has a 202 that defines a non-empty "final schema". I wanted a "real" test case so I rummaged through the azure-powershell and found:

https://github.com/Azure/azure-rest-api-specs/blob/fbb4e2c74897a67a75116d2a3157bd146262def4/specification/cost-management/resource-manager/Microsoft.CostManagement/stable/2022-05-01/costmanagement.generatecostdetailsreport.json

in src/CostManagement/README.md, and specifically this response.

Then I checked to see how the new autorest versions affect generation, so I generated with the default configuration:



autorest --powershell --use:@autorest/powershell@4.x --input-file:https://github.com/Azure/azure-rest-api-specs/blob/fbb4e2c74897a67a75116d2a3157bd146262def4/specification/cost-management/resource-manager/Microsoft.CostManagement/stable/2022-05-01/costmanagement.generatecostdetailsreport.json --output-folder:baseline

and again with the updated autorest/modelerfour versions

autorest --powershell --use:@autorest/powershell@4.x --input-file:https://github.com/Azure/azure-rest-api-specs/blob/fbb4e2c74897a67a75116d2a3157bd146262def4/specification/cost-management/resource-manager/Microsoft.CostManagement/stable/2022-05-01/costmanagement.generatecostdetailsreport.json --use:@autorest/powershell@4.x --use:@autorest/modelerfour@4.26.2 --version:3.9.6 --output-folder:updated

and compared the results:



diff -r baseline updated   # shows no changes

Next I created a branch in my fork of azure-rest-api-specs rooted at the fbb4e2c74897a67a75116d2a3157bd146262def4 commit and made one commit to remove the 200 response. Generating from this file shows many changes.

autorest --powershell --use:@autorest/powershell@4.x --input-file:https://github.com/mikekistler/azure-rest-api-specs/blob/e35f4dc958b2aade53b5d8c2e559b3805fd39720/specification/cost-management/resource-manager/Microsoft.CostManagement/stable/2022-05-01/costmanagement.generatecostdetailsreport.json --use:@autorest/powershell@4.x --use:@autorest/modelerfour@4.26.2 --version:3.9.6 --output-folder:mk-try1

diff -r baseline mk-try1    # shows many changes

This just demonstrates that the 200 response is significant. Then I made one more commit to add the final-state-schema and generated again from this new file.

autorest --powershell --use:@autorest/powershell@4.x --input-file:https://github.com/mikekistler/azure-rest-api-specs/blob/c412be4e362079cfbaf46a8ba0cfd1f4044efca2/specification/cost-management/resource-manager/Microsoft.CostManagement/stable/2022-05-01/costmanagement.generatecostdetailsreport.json --use:@autorest/powershell@4.x --use:@autorest/modelerfour@4.26.2 --version:3.9.6 --output-folder:mk-try2

With final-state-schema added, the generated code is again exactly the same as the baseline.

diff -r baseline mk-try2    # shows no changes

So I think this is working for powershell generation. What do you think?

dolauli commented 1 year ago

@mikekistler Thanks for confirming that autorest.powershell works for your case. Upon comparing my case with yours, I noticed that the difference lies in my case where both the 200/204 responses and final-state-schema are defined. It seems that when 200/204 is defined in the swagger, modelerfour ignores the final-state-schema. I'm unsure if this behavior is expected or if my case is invalid.

Additionally, I have two questions:

  1. Is there a centralized documentation for the changes related to LRO (final-state-schema, response changes for post/put/patch/delete)?
  2. Do you have a sample swagger that covers different cases and can be used to test different generators for this change?
mikekistler commented 1 year ago

It is intentional for the 200 schema to take precedence over final-state-schema. We chose this design since the 200 response is already the established pattern.

The documentation for final-state-schema is in the autorest extensions documentation. I'd be happy for feedback/suggestions on how to improve that.

I have been testing the other generators using the Monitor service, but I discovered that powershell does not generate a command for the particular operation I was using, so I had to pick another service (cost management). My hope was to modify the API to use final-state-schema and show that it generates the same result as already shipped in azure-powershell, but I could not figure out how to reproduce the shipped version (even without adding final-state-schema).

dolauli commented 1 year ago

Regarding autorest.powershell version 4, I've implemented the necessary change here to upgrade the dependency of modelerfour to the latest 4.26.x version, which supports final-state-schema.

To use v4, you can add the following configuration in the README.md file or specify --use:@autorest/powershell@4.x in the command line:

use-extension:
  "@autorest/powershell": "4.x"

As for autorest.powershell version 3, no further changes will be made, as it is now in maintenance mode and will not receive new features. For v3, a workaround is to specify the modelerfour version in the command line by adding --use:@autorest/modelerfour@4.26.2 or adding the following configuration in README.md:

use-extension:
  "@autorest/modelerfour": "4.26.2"

Last, if you have ever used autorest.powershell before, you will need to run autorest --reset to clean local cached old versions of autorest.powershell.

dolauli commented 1 year ago

My hope was to modify the API to use final-state-schema and show that it generates the same result as already shipped in azure-powershell, but I could not figure out how to reproduce the shipped version (even without adding final-state-schema).

@mikekistler, just to clarify, in Azure PowerShell, we are responsible for maintaining the configurations (README.md) and custom code of different modules in our repository (https://github.com/Azure/azure-powershell). For example, if you are looking for the Databricks module, you can visit https://github.com/Azure/azure-powershell/tree/autorest/developer/src/Databricks and run autorest --use:@autorest/powershell@4.x in that directory.

I believe I can conduct the same tests that you are working on. However, I will need you to provide me with the details on how to modify different APIs (post/put/patch/delete) to use final-state-schema.

Regarding the documentation, I understand that most people are interested in understanding how the swagger of LRO APIs for patch/put/delete/post used to be, and what changes are required after the addition of final-state-schema. Having clear documentation on these modifications will be valuable for everyone's understanding.