Azure
Adding 2023 09 01 for nginx
ARM (Control Plane) API Specification Update Pull Request
Tip: overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.
PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
Click here to see the details of Step 1
Breaking changes review (Diagram Step 1)
If the automation determines you have breaking changes, i.e. Step 1 from the diagram applies to you,
you must follow the breaking changes process.
IMPORTANT This applies even if:
- The tool fails while it shouldn't, e.g. due to runtime exception, or incorrect detection of breaking changes.
- You believe there is no need for you to request breaking change approval, for any reason. Such claims must be reviewed, and the process is the same.
Click here to see the details of Step 2
ARM API changes review (Diagram Step 2)
- If this PR is in purview of ARM review then automation will add the
ARMReviewlabel. - If you want to force ARM review, add the label yourself.
- Proceed according to the diagram at the top of this comment.
Click here to see the diagram footnotes
Diagram footnotes
[1] ARM review queue (for merge queues, see [2])
The PRs are processed by time opened, ascending. Your PR may show up on 2nd or later page.
If you addressed Step 1 from the diagram and your PR is not showing up in the queue, ensure the label ARMChangesRequested
is removed from your PR. This should cause the label WaitForARMFeedback to be added.
[2] public repo merge queue, private repo merge queue (for ARM review queue, [1])
If you need further help with anything, see Getting help section below.
Purpose of this PR
What's the purpose of this PR? Check all that apply. This is mandatory!
- [x] New API version. (If API spec is not defined in TypeSpec, the PR should have been generated using OpenAPI Hub).
- [ ] Update existing version for a new feature. (This is applicable only when you are revising a private preview API version.)
- [ ] Update existing version to fix swagger quality issues in S360.
- [ ] Other, please clarify:
- edit this with your clarification
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood and followed the instructions by checking all the boxes:
- [x] I confirm this PR is modifying Azure Resource Manager (ARM) related specifications, and not data plane related specifications.
- [x] I have reviewed following Resource Provider guidelines, including
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the Diagram Step 2, "ARM API changes Review", for this PR.
Additional information
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.
Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the Swagger-Suppression-Process to get approval.
Getting help
- First, please carefully read through this PR description, from top to bottom. Please fill out the
Purpose of this PRandDue diligence checklist. - To understand what you must do next to merge this PR, see the
Next Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state. - For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure and https://aka.ms/ci-fix.
- If the PR CI checks appear to be stuck in
queuedstate, please add a comment with contents/azp run. This should result in a new comment denoting aPR validation pipelinehas started and the checks should be updated after few minutes. - If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.
Next Steps to Merge
✔️ All automated merging requirements have been met! Refer to step 4 in the PR workflow diagram (even if your PR is for data plane, not ARM).![openapi-pipeline-app[bot] avatar](https://avatars.githubusercontent.com/in/65541?v=4 "Published by a pub.dev verified publisher"){kind=small}
Swagger Validation Report
️️✔️BreakingChange succeeded [Detail] [Expand]
There are no breaking changes.
️❌Breaking Change(Cross-Version): 17 Errors, 25 Warnings failed [Detail]
| compared swaggers (via Oad v0.10.4)] | new version | base version |
|---|---|---|
| swagger.json | 2023-09-01(2ad42fd) | 2023-04-01(main) |
| swagger.json | 2023-09-01(2ad42fd) | 2021-05-01-preview(main) |
The following breaking changes are detected by comparison with the latest stable version:
The following breaking changes are detected by comparison with the latest preview version:
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️LintDiff: 4 Warnings warning [Detail]
| compared tags (via openapi-validator v2.1.6) | new version | base version |
|---|---|---|
| package-2023-09-01 | package-2023-09-01(2ad42fd) | default(main) |
[must fix]The following errors/warnings are introduced by current PR:
| Rule | Message | Related RPC [For API reviewers] |
|---|---|---|
| :warning: PostOperationIdContainsUrlVerb | OperationId should contain the verb: 'analyze' in:'Configurations_Analysis'. Consider updating the operationId Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L485 |
|
| :warning: SchemaDescriptionOrTitle | Schema should have a description or title. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L941 |
|
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L1469 |
|
| :warning: AvoidNestedProperties | Consider using x-ms-client-flatten to provide a better end user experience Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L1511 |
The following errors/warnings exist before current PR submission:
Only 30 items are listed, please refer to log for more details.
| Rule | Message |
|---|---|
|
The resource name parameter 'certificateName' should be defined with a 'pattern' restriction. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L29 |
|
A tracked resource MUST always have tags as a top level optional property. Tracked resource does not have tags in the request schema. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L80 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L137 |
|
Long-running delete operations must have responses with 202, 204 and default return codes. They also must have no other response codes. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L151 |
|
A 202 response should include an Location response header. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L185 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L194 |
|
The resource name parameter 'configurationName' should be defined with a 'pattern' restriction. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L302 |
|
A tracked resource MUST always have tags as a top level optional property. Tracked resource does not have tags in the request schema. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L353 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L410 |
|
Long-running delete operations must have responses with 202, 204 and default return codes. They also must have no other response codes. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L424 |
|
A 202 response should include an Location response header. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L458 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L467 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L636 |
|
Long-running PATCH operations must have responses with 200, 202 and default return codes. They also must not have other response codes. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L650 |
|
Mark the top-level property 'location', specified in the patch operation body, as readOnly or immutable. You could also choose to remove it from the request payload of the Patch operation. This property is not patchable. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L675 |
|
The async patch operation should return 202. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L683 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L699 |
|
Long-running delete operations must have responses with 202, 204 and default return codes. They also must have no other response codes. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L710 |
|
A 202 response should include an Location response header. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L737 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L746 |
|
Operations API path must follow the schema provided in the common types. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L864 |
|
Tracked resource 'NginxCertificate' must have patch operation that at least supports the update of tags. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L952 |
|
Tracked resource 'NginxConfiguration' must have patch operation that at least supports the update of tags. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L1048 |
| :warning: LatestVersionOfCommonTypesMustBeUsed | Use the latest version v5 of types.json. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L41 |
| :warning: LatestVersionOfCommonTypesMustBeUsed | Use the latest version v5 of types.json. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L44 |
| :warning: LatestVersionOfCommonTypesMustBeUsed | Use the latest version v5 of types.json. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L57 |
| :warning: LatestVersionOfCommonTypesMustBeUsed | Use the latest version v5 of types.json. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L94 |
| :warning: LatestVersionOfCommonTypesMustBeUsed | Use the latest version v5 of types.json. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L97 |
| :warning: LatestVersionOfCommonTypesMustBeUsed | Use the latest version v5 of types.json. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L110 |
| :warning: LatestVersionOfCommonTypesMustBeUsed | Use the latest version v5 of types.json. Location: NGINX.NGINXPLUS/stable/2023-09-01/swagger.json#L162 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
️🔄ApiDocPreview inProgress [Detail]
️❌SDK Breaking Change Tracking failed [Detail]
Breaking Changes Tracking
❌azure-sdk-for-go - sdk/resourcemanager/nginx/armnginx - Approved - 4.0.0+ Struct `ErrorResponseBody` has been removed + Type of `ResourceProviderDefaultErrorResponse.Error` has been changed from `*ErrorResponseBody` to `*ErrorDetail`
❌azure-sdk-for-js - @azure/arm-nginx - Approved - 4.0.0+ Type of parameter error of interface ResourceProviderDefaultErrorResponse is changed from ErrorResponseBody to ErrorDetail
️️✔️ azure-sdk-for-net succeeded [Detail] [Expand]
️✔️Succeeded [Logs] Generate from c8e8798c8671f931ff9b05be84121f8394368466. SDK Automation 14.0.0command pwsh ./eng/scripts/Automation-Sdk-Init.ps1 ../azure-sdk-for-net_tmp/initInput.json ../azure-sdk-for-net_tmp/initOutput.json command autorest --version=2.0.4421 --csharp --reflect-api-versions --license-header=MICROSOFT_MIT_NO_VERSION [email protected]/[email protected] --csharp-sdks-folder=/mnt/vss/_work/1/s/azure-sdk-for-net/sdk ../azure-rest-api-specs/specification/nginx/resource-manager/readme.md cmderr [Autorest] realpath(): Permission denied cmderr [Autorest] realpath(): Permission denied cmderr [Autorest] realpath(): Permission denied
️✔️Microsoft.Azure.Management.Nginx [View full logs] [Preview SDK Changes]warn Skip artifact folder because it doesn't exist: artifacts/packages
️⚠️ azure-sdk-for-python-track2 warning [Detail]
⚠️Warning [Logs] Generate from c8e8798c8671f931ff9b05be84121f8394368466. SDK Automation 14.0.0command sh scripts/automation_init.sh ../azure-sdk-for-python_tmp/initInput.json ../azure-sdk-for-python_tmp/initOutput.json cmderr [automation_init.sh] WARNING: azure-devtools 1.2.1 does not provide the extra 'ci-tools' cmderr [automation_init.sh] WARNING: azure-devtools 1.2.1 does not provide the extra 'ci-tools' cmderr [automation_init.sh] WARNING: Skipping azure-nspkg as it is not installed. command sh scripts/automation_generate.sh ../azure-sdk-for-python_tmp/generateInput.json ../azure-sdk-for-python_tmp/generateOutput.json cmderr [automation_generate.sh] cmderr [automation_generate.sh] New minor version of npm available! 10.2.3 -> 10.3.0 cmderr [automation_generate.sh] npm notice Changelog: <https://github.com/npm/cli/releases/tag/v10.3.0> cmderr [automation_generate.sh] npm notice Run `npm install -g [email protected]` to update! cmderr [automation_generate.sh] npm notice
️✔️track2_azure-mgmt-nginx [View full logs] [Preview SDK Changes]info [Changelog] ### Features Added info [Changelog] info [Changelog] - Added operation ConfigurationsOperations.analysis info [Changelog] - Model NginxCertificateProperties has a new parameter certificate_error info [Changelog] - Model NginxCertificateProperties has a new parameter key_vault_secret_created info [Changelog] - Model NginxCertificateProperties has a new parameter key_vault_secret_version info [Changelog] - Model NginxCertificateProperties has a new parameter sha1_thumbprint
️⚠️ azure-sdk-for-java warning [Detail]
⚠️Warning [Logs] Generate from c8e8798c8671f931ff9b05be84121f8394368466. SDK Automation 14.0.0command ./eng/mgmt/automation/init.sh ../azure-sdk-for-java_tmp/initInput.json ../azure-sdk-for-java_tmp/initOutput.json cmderr [init.sh] [notice] A new release of pip is available: 23.0.1 -> 23.3.2 cmderr [init.sh] [notice] To update, run: pip install --upgrade pip cmderr [init.sh] [notice] A new release of pip is available: 23.0.1 -> 23.3.2 cmderr [init.sh] [notice] To update, run: pip install --upgrade pip cmderr [init.sh] rent cmderr [init.sh] Dload Upload Total Spent Left Speed cmderr [init.sh] 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 095k cmderr [init.sh] Downloading https://nodejs.org/dist/v18.15.0/node-v18.15.0-linux-x64.tar.xz... cmderr [init.sh] ############# 100.0% cmderr [init.sh] Computing checksum with sha256sum cmderr [init.sh] Checksums matched! command ./eng/mgmt/automation/generate.py ../azure-sdk-for-java_tmp/generateInput.json ../azure-sdk-for-java_tmp/generateOutput.json
️✔️azure-resourcemanager-nginx [View full logs] [Preview SDK Changes]
️️✔️ azure-sdk-for-go succeeded [Detail] [Expand]
️✔️Succeeded [Logs] Generate from c8e8798c8671f931ff9b05be84121f8394368466. SDK Automation 14.0.0command sh ./eng/scripts/automation_init.sh ../../../../../azure-sdk-for-go_tmp/initInput.json ../../../../../azure-sdk-for-go_tmp/initOutput.json command generator automation-v2 ../../../../../azure-sdk-for-go_tmp/generateInput.json ../../../../../azure-sdk-for-go_tmp/generateOutput.json
️✔️sdk/resourcemanager/nginx/armnginx [View full logs] [Preview SDK Changes] Breaking Change Detectedinfo [Changelog] ### Breaking Changes info [Changelog] info [Changelog] - Type of `ResourceProviderDefaultErrorResponse.Error` has been changed from `*ErrorResponseBody` to `*ErrorDetail` info [Changelog] - Struct `ErrorResponseBody` has been removed info [Changelog] info [Changelog] ### Features Added info [Changelog] info [Changelog] - New function `*ConfigurationsClient.Analysis(context.Context, string, string, string, *ConfigurationsClientAnalysisOptions) (ConfigurationsClientAnalysisResponse, error)` info [Changelog] - New struct `AnalysisCreate` info [Changelog] - New struct `AnalysisCreateConfig` info [Changelog] - New struct `AnalysisError` info [Changelog] - New struct `AnalysisResult` info [Changelog] - New struct `AnalysisResultData` info [Changelog] - New struct `CertificateErrorResponseBody` info [Changelog] - New struct `ErrorAdditionalInfo` info [Changelog] - New struct `ErrorDetail` info [Changelog] - New field `CertificateError`, `KeyVaultSecretCreated`, `KeyVaultSecretVersion`, `SHA1Thumbprint` in struct `CertificateProperties` info [Changelog] info [Changelog] Total 3 breaking change(s), 18 additive change(s).
️️✔️ azure-sdk-for-js succeeded [Detail] [Expand]
️✔️Succeeded [Logs] Generate from c8e8798c8671f931ff9b05be84121f8394368466. SDK Automation 14.0.0command sh .scripts/automation_init.sh ../azure-sdk-for-js_tmp/initInput.json ../azure-sdk-for-js_tmp/initOutput.json warn File azure-sdk-for-js_tmp/initOutput.json not found to read command sh .scripts/automation_generate.sh ../azure-sdk-for-js_tmp/generateInput.json ../azure-sdk-for-js_tmp/generateOutput.json
️✔️@azure/arm-nginx [View full logs] [Preview SDK Changes] Breaking Change Detectedinfo [Changelog] **Features** info [Changelog] info [Changelog] - Added operation Configurations.analysis info [Changelog] - Added Interface AnalysisCreate info [Changelog] - Added Interface AnalysisCreateConfig info [Changelog] - Added Interface AnalysisError info [Changelog] - Added Interface AnalysisResult info [Changelog] - Added Interface AnalysisResultData info [Changelog] - Added Interface ConfigurationsAnalysisOptionalParams info [Changelog] - Added Interface ErrorAdditionalInfo info [Changelog] - Added Interface ErrorDetail info [Changelog] - Added Interface NginxCertificateErrorResponseBody info [Changelog] - Added Type Alias ConfigurationsAnalysisResponse info [Changelog] - Interface NginxCertificateProperties has a new optional parameter certificateError info [Changelog] - Interface NginxCertificateProperties has a new optional parameter keyVaultSecretCreated info [Changelog] - Interface NginxCertificateProperties has a new optional parameter keyVaultSecretVersion info [Changelog] - Interface NginxCertificateProperties has a new optional parameter sha1Thumbprint info [Changelog] info [Changelog] **Breaking Changes** info [Changelog] info [Changelog] - Type of parameter error of interface ResourceProviderDefaultErrorResponse is changed from ErrorResponseBody to ErrorDetail
️⚠️ azure-resource-manager-schemas warning [Detail]
⚠️Warning [Logs] Generate from c8e8798c8671f931ff9b05be84121f8394368466. Schema Automation 14.0.0command .sdkauto/initScript.sh ../azure-resource-manager-schemas_tmp/initInput.json ../azure-resource-manager-schemas_tmp/initOutput.json cmderr [initScript.sh] notice cmderr [initScript.sh] npm notice New minor version of npm available! 10.2.3 -> 10.3.0 cmderr [initScript.sh] npm notice Changelog: <https://github.com/npm/cli/releases/tag/v10.3.0> cmderr [initScript.sh] npm notice Run `npm install -g [email protected]` to update! cmderr [initScript.sh] npm notice warn File azure-resource-manager-schemas_tmp/initOutput.json not found to read command .sdkauto/generateScript.sh ../azure-resource-manager-schemas_tmp/generateInput.json ../azure-resource-manager-schemas_tmp/generateOutput.json
️✔️nginx [View full logs] [Preview Schema Changes]
️️✔️ azure-powershell succeeded [Detail] [Expand]
️✔️Succeeded [Logs] Generate from c8e8798c8671f931ff9b05be84121f8394368466. SDK Automation 14.0.0command sh ./tools/SwaggerCI/init.sh ../azure-powershell_tmp/initInput.json ../azure-powershell_tmp/initOutput.json command pwsh ./tools/SwaggerCI/psci.ps1 ../azure-powershell_tmp/generateInput.json ../azure-powershell_tmp/generateOutput.json
️✔️Az.nginx.DefaultTag [View full logs] [Preview SDK Changes]
Generated ApiView
| Language | Package Name | ApiView Link |
|---|---|---|
| Go | sdk/resourcemanager/nginx/armnginx | https://apiview.dev/Assemblies/Review/54b1e1dfd98c4c2cb44c0a03efa8326f?revisionId=a7741162efab42b7934677e609a1a871 |
| JavaScript | @azure/arm-nginx | https://apiview.dev/Assemblies/Review/58527f71fd1645209a788a5fd37fb991?revisionId=99d0031ed3af4298bf5d84470282ac1b |
| Java | azure-resourcemanager-nginx | https://apiview.dev/Assemblies/Review/01464d26eed94c4badee1327960c1323?revisionId=c5894f4838d84becaa5734725fb3e88d |
| Swagger | NGINX.NGINXPLUS | https://apiview.dev/Assemblies/Review/4ed35686973f43ffa48dfbe37535befb?revisionId=7aabf55fe43f433d974f6279d849fd19 |
Sync version from private to public. Code updates are exactly same as it is in private repo here: https://github.com/Azure/azure-rest-api-specs-pr/tree/RPSaaSMaster/specification/nginx/resource-manager/NGINX.NGINXPLUS/stable/2023-09-01
Please address or respond to feedback from the ARM API reviewer.
When you are ready to continue the ARM API review, please remove the ARMChangesRequested label.
This will notify the reviewer to have another look.
If the feedback provided needs further discussion, please use this Teams channel to post your questions - aka.ms/azsdk/support/specreview-channel.
Please include [ARM Query] in the title of your question to indicate that it is ARM-related.
![openapi-workflow-bot[bot] avatar](https://avatars.githubusercontent.com/in/81125?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @zaowang-ms! For review efficiency consideration, when creating a new API version, it is required to place API specs of the base version in the first commit, and push new version updates into successive commits. You can use OpenAPIHub to initialize the PR for adding a new version.
For more details refer to the wiki.
Fixed commit history to make 2023-04-01 as first commit, and 2023-09-01 as follows
Breaking changes previously reviewed and approved in https://github.com/Azure/azure-rest-api-specs-pr/pull/15270
Hi @mentat9, thanks for removing the tag for me. Do we need separate approvals for each SDK breaking change, or is the main one enough?
Hello @mentat9,
Thank you for your thorough and insightful reviews, especially regarding "x-ms-identifiers" and "id."
We're currently at a stage where implementing changes is particularly challenging. Our workflow is structured such that modifications aren't anticipated during the synchronization from azure-rest-api-specs-pr to azure-rest-api-specs. This stage is critical for finalizing content that is already set to be released to the public.
Incorporating changes at this point would necessitate a return to the azure-rest-api-specs-pr stage, followed by initiating an entirely new PR. This process deviation could significantly delay our current release timeline. Also, introducing changes now could result in breaking changes for our customers, which is a critical concern, particularly regarding the verb analyze issue.
Given these constraints and the discussions about our workflow, we kindly request your approval to proceed with the current PR as is. We highly value your feedback and intend to include many of your suggestions in our next stable version.
Also, below is a workflow of our API changes, based on our team's understanding of the api change workflow and accommodated with our business need to work with partners. Please let us know if this workflow is incorrect, especially the part where we do not anticipate changes during the synchronization from azure-rest-api-specs-pr to azure-rest-api-specs. Muchly appreciated!
@zaowang-ms - If these changes were already reviewed and signed off by ARM in the private repo, just provide a link to the PR and I'll wave it through. We don't need to review your changes multiple times (especially if they are just moving from private to public repo).
Your flowchart above looks correct to me. In future, feel free to alert the ARM reviewer up front (including a link to the PR) if a change has already been reviewed and signed off by ARM in a different PR.
@mentat9 Thanks for replying! Yes, the changes were all approved and signedoff before when it was merged in the private repo. These are the two relevant PRs that included these changes:
https://github.com/Azure/azure-rest-api-specs-pr/pull/14498
https://github.com/Azure/azure-rest-api-specs-pr/pull/15270
I did mention it but should have provide these links earlier to reduce misunderstanding. Sorry for any delay and extra work caused by that, and thanks fir reviewing our workflow as well. Please let me know if anything else needed. Thanks!
@zaowang-ms - Signing off for ARM per signoff in the private repo. For future APIs, please use verbs for POST actions.
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @mentat9 , thanks for the sign off. It's definitely something we should be careful of. Thanks again!
Notes for SDK reviewers:
The identified sdk breaking changes were all approved and signed off before when it was merged in the private repo. These are the two relevant PRs that included these changes:
https://github.com/Azure/azure-rest-api-specs-pr/pull/14498
https://github.com/Azure/azure-rest-api-specs-pr/pull/15270
As we've received ARMSignedOfff, I think we are awaiting SDK reviews now. Please let us know if any other information is needed from us. Thanks!
@rkmanda is there any extra approval needed for this PR? I've requested merge and there is no update yet for a day. It would be great if you can help to take a look on this, thanks!
@mentat9 I still have some concerns about the verb thing. Waving it through means we'll never be able to fix it to a verb without it being a breaking change. And properly using verbs instead of nouns is actually quite useful to customers, for being able to understand the intent of the API - or also the meaning of the related RBAC action!
I wouldn't say there is really anything wrong with the workflow, but just has a challenge here and now because of the reality that API review is neither perfect nor perfectly consistent.
Are we all okay with keeping it as a noun forever, in order to avoid breaking changes, just because of this workflow constraint? And is the only reason not to fix it only a convenience for ourselves? If so, I tend to think we should fix that one change in private-specs PR first (smallest possible fix), and then sync it here. I am also curious what some leaders think. /cc @JeffreyRichter @rkmanda
@mentat9 I still have some concerns about the verb thing. Waving it through means we'll never be able to fix it to a verb without it being a breaking change. And properly using verbs instead of nouns is actually quite useful to customers, for being able to understand the intent of the API - or also the meaning of the related RBAC action!
I wouldn't say there is really anything wrong with the workflow, but just has a challenge here and now because of the reality that API review is neither perfect nor perfectly consistent.
Are we all okay with keeping it as a noun forever, in order to avoid breaking changes, just because of this workflow constraint? And is the only reason not to fix it only a convenience for ourselves? If so, I tend to think we should fix that one change in private-specs PR first (smallest possible fix), and then sync it here. I am also curious what some leaders think. /cc @JeffreyRichter @rkmanda
I agree: that's the main thing that concerned me as well.
The same feedback was given on the original PR: https://github.com/Azure/azure-rest-api-specs-pr/pull/15270/files#r1366341718, but was never acted on (or responded to). Unless there was an agreement on some other channel, it seems like this one just slipped through the cracks.
I'm also interested in input from others (@JeffreyRichter, @mikekistler , @rkmanda).
Thanks for your insights @mentat9 and @TimLovellSmith. I'd like to share some thoughts regarding this:
- The initial reason for using a noun in our API paths was to minimize differences with downstream APIs, specifically inheriting the path "analysis" provided by our partner.
- Why is it not responded at first place? If you examine the history of our private PR at https://github.com/Azure/azure-rest-api-specs-pr/pull/15270, you'll notice that the verb naming issue was raised after the PR was approved and merged. I think that at that time, the review queue was very long, and both the PR creators and reviewers were burdened, likely resulting in this oversight.
- Despite this, the point about verb naming in the API is valid. There are only 2 main costs: 1. There will be a product breaking change as it’s already in use on Azure portal. 2. A delay of several weeks in introducing new features and their business impact.
Considering our discussion, we have two main options:
- Revise the API to align with previous standards, collaborating closely to speed up the review and minimize associated costs.
- Postpone this change, and plan for a breaking change in a future API version, reducing immediate business impact.
In short, we must balance the need for consistency with partner APIs and the efficiency of our review process. The choice between immediate revision or future planning will depend on how we prioritize these aspects in our API development strategy.
@zaowang-ms so from your side, you'd like to go ahead with the merge as-is?
@zaowang-ms so from your side, you'd like to go ahead with the merge as-is?
Hi @TimLovellSmith , I think @JeffreyRichter commented as follows this morning and it didn't show up in the thread somehow:
It does seem like "analysis" should be "analyze" especially since the summary starts with "Analyze an NGINX configuration..." so the URL isn't consistent with its own documentation. So, I agree this should be a noun.
If a version of the service has already shipped with "analysis" then changing it is a breaking change and we should not do it if any 3rd party customers could be using it. If the only customers are 1st party (like the Azure Portal), then we don't care about breaking 1st party as we can work with them closely to fix things without breaking customers and we always compromise 1st party to make a better long-term customer (3rd party) experience.
Also, we almost always compromise immediate negative business impact in favor of the long-term positive customer experience. If we could fix this now by introducing some delay (negative business impact) but not have to break customers in the future, then we should definitely do this. Breaking customers causes severe customer dissatisfaction with Azure all up - the breaking change board was created a few years ago within the Azure org with the sole charter of reducing all the breaks to improve overall Azure customer satisfaction.
I appreciate Jeffrey's perspective, emphasizing long-term customer experience over immediate business concerns. We do initially prefer to proceed directly regarding the potential business cost as we work with F5 NGINX to expand the features of NGINXaaS for Azure quickly. However, now by understanding Jeffrey's points, we can adapt to this change and will communicate with our partners accordingly.
There are currently no 3rd party customers as the Api is still not public. I have created the PR to fix that: https://github.com/Azure/azure-rest-api-specs-pr/pull/16675 This would be a breaking change and need approval again. Please allow me to ask for an expedited process on approve this one, if possible, to reduce impact on feature delivery schedules and 1st services. Thanks!
I can remove mergeRequested for now until we fixed the other PR then, I think, then we can add it back.
PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.