azure-rest-api-specs
azure-rest-api-specs copied to clipboard
Azure
[Azure DevCenter] Update operation groups to improve SDK codegen
Data Plane API - Pull Request
Updating operation grouping and specifying method location for SDK generation, based on guidance from the SDK Architecture Board. These updates do not impact any APIs, only operation groups and parameter location for one parameter to improve generated code.
API Info: The Basics
- Link to engagement record issue: N/A
Is this review for (select one):
- [ ] a private preview
- [x] a public preview
- [ ] GA release
Change Scope
- Updates to operation groups and parameter location for SDK generation
- Removing two $top parameters from GET APIs. These were mistakenly added, and our API has never supported $top for any GET APIs.
Updates to operation IDs are to group use-cases by client, per guidance from the SDK board. Updates to ProjectNameParameter are to indicate that in one client this will be a method parameter, while on the other clients it will be a client parameter because those clients will not act across projects. Again, this will only impact codegen. Updates to $top parameters are to reflect the actual support of the API and remove the parameter from generated code.
Tooling
- Open API validation tools were run on this PR. Go here to see how to fix errors
- Spectral Linting
- Open API Hub
Guidelines & Specifications
Helpful Links
Hi, @chrissmiller Thanks for your PR. I am workflow bot for review process. Here are some small tips.
![openapi-workflow-bot[bot] avatar](https://avatars.githubusercontent.com/in/81125?v=4 "Published by a pub.dev verified publisher"){kind=small}
Swagger Validation Report
️❌BreakingChange: 32 Errors, 0 Warnings failed [Detail]
| compared swaggers (via Oad v0.9.7)] | new version | base version |
|---|---|---|
| 2022-03-01-preview | 2022-03-01-preview(75a8d8d) | 2022-03-01-preview(main) |
Only 30 items are listed, please refer to log for more details.
️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️LintDiff succeeded [Detail] [Expand]
Validation passes for LintDiff.
| compared tags (via openapi-validator v1.13.0) | new version | base version |
|---|---|---|
| package-2022-03-01-preview | package-2022-03-01-preview(75a8d8d) | package-2022-03-01-preview(main) |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ApiReadinessCheck succeeded [Detail] [Expand]
️️✔️~[Staging] ServiceAPIReadinessTest succeeded [Detail] [Expand]
Validation passes for ServiceAPIReadinessTest.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SDK Track2 Validation succeeded [Detail] [Expand]
Validation passes for SDKTrack2Validation
- The following tags are being changed in this PR
- "https://github.com/Azure/azure-rest-api-specs/blob/75a8d8dcc9f6d0ec626bdeb32f5154f20c8c61cd/specification/devcenter/data-plane/readme.md#tag-package-2022-03-01-preview">devcenter/data-plane/readme.md#package-2022-03-01-preview
️️✔️PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️CadlValidation succeeded [Detail] [Expand]
Validation passes for CadlValidation.
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
![openapi-pipeline-app[bot] avatar](https://avatars.githubusercontent.com/in/65541?v=4 "Published by a pub.dev verified publisher"){kind=small}
Swagger pipeline restarted successfully, please wait for status update in this comment.
Swagger pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.
Hi @chrissmiller, Your PR has some issues. Please fix the CI sequentially by following the order of Avocado, semantic validation, model validation, breaking change, lintDiff. If you have any questions, please post your questions in this channel https://aka.ms/swaggersupport.
| Task | How to fix | Priority |
|---|---|---|
| Avocado | Fix-Avocado | High |
| Semantic validation | Fix-SemanticValidation-Error | High |
| Model validation | Fix-ModelValidation-Error | High |
| LintDiff | Fix-LintDiff | high |
Hi @chrissmiller, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. Action: To initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki. If you want to know the production traffic statistic, please see ARM Traffic statistic. If you think it is false positive breaking change, please provide the reasons in the PR comment, report to Swagger Tooling Team via https://aka.ms/swaggerfeedback. Note: To avoid breaking change, you can refer to Shift Left Solution for detecting breaking change in early phase at your service code repository.
Hi @jhendrixMSFT ,
I've provided justification below for the changes. I'm not certain if this is a valid breaking change flag -- the removed top parameters were never valid/accepted by our service, since they are on GET operations, and the operation ID breaking change is for generated code, but we are basing/releasing our generated SDKs off of the new operation IDs from this PR, per request from the SDK board, so I don't see any potential customer impact from these changes. Please let me know if this requires filing an issue, and if the process is different given that we are in preview.
Justification:
- Top parameter removal: Both these top parameters are on GET operations. They were mistakenly included in the spec, and have never been accepted by our API. Our service has always ignored these parameters, so removal results in no change for customers, and use of the parameters is not a valid scenario (since the GET will always return 1 or 0 items). I have confirmed via telemetry that the
topparameter on these two APIs has never been used by customers (or internal users for that matter). - Operation ID renaming: These changes were requested by the SDK Architecture board. They asked that we restructure our operation IDs to better follow convention and to group operations into logical client groupings, rather than grouping by resource to improve customer experience with languages that produce multiple clients (such as .NET). This spec will be used for our initial SDK releases, and we have not previously generated any code using the old spec (so there are no related breaking changes for Azure SDKs released by our service).
We also have a linting issue from the new operation IDs. This is because with the clients encompassing multiple resources, so an operation ID such as Environments_GetEnvironment is required to explicitly show the resource being fetched, and distinguish it from other GETs on the same client such as Environments_GetCatalogItem. This is based off of the SDK architecture board guidance on naming. Please let me know if there is any issue here.
None of the SDK is released, hence changing on "OperationId" should be fine (as long as SDK arch board is fine with their SDK).
However the removal of "top" parameter would require approval from Jeffrey. Please see https://github.com/Azure/azure-rest-api-specs/pull/21086#issuecomment-1276540988. Maybe use the office hour.
I can approve the breaking change about top since the nservice supported it. The problem here is that top gets documented and SDKs expose it. If top then gets removed, then customer code that tried to set top is now broken. For some SDK languages (like Go), this requires a major version change and then we have a very hard time getting customers to adopt it as they have to change all their source code files to import the new major version. The service team MUST validate their swaggers better before we publish them to avoid these gratuitous breaking changes which impact docs, SDKs, and tools (PowerShell/CLI) - the ripple effect of these breaks is enormous.
Thanks Jeffrey and Weidong -- confirming that none of the SDKs have been released yet (and all the ones in review are based off of this updated spec). We have taken this as an opportunity to improve our review process for our spec, and introduced some additional gates to pull requests with spec changes during spec development to prevent issues like the mistaken top param from being introduced into future API versions.