-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NetAppFiles] Anf 27235 update for 2022-11-01-preview #24741
[NetAppFiles] Anf 27235 update for 2022-11-01-preview #24741
Conversation
Hi, @audunn! Thank you for your pull request. To help get your PR merged: Generated ApiView comment added to this PR. You can use ApiView to show API versions diff. |
Swagger Generation Artifacts
|
Generated ApiView
|
Please address or respond to feedback from the ARM API reviewer. |
Hi @audunn! The automation detected breaking changes in this pull request. As a result, it added the |
Hi @audunn! Your PR has some issues. Please fix the CI issues, if present, in following order:
If you need further help, please reach out on the Teams channel aka.ms/azsdk/support/specreview-channel. |
Hi @audunn! 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. |
Breaking change was approved here https://msazure.visualstudio.com/One/_workitems/edit/15313053/ |
Or did you just have to remove properties because of flattening meaning its not actually even a thing in the payload? In reply to: 1679507476 Refers to: specification/netapp/resource-manager/Microsoft.NetApp/preview/2022-11-01-preview/netapp.json:7477 in 801f0ea. [](commit_id = 801f0ea, deletion_comment = True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@rkmanda are we good to have this merged ? |
/pr RequestMerge |
Next Steps to Merge |
- $.paths["/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.NetApp/netAppAccounts/{accountName}/backupVaults/{backupVaultName}/backups/{backupName}/restoreFiles"].post | ||
- $.paths["/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.NetApp/netAppAccounts/{accountName}/capacityPools/{poolName}/volumes/{volumeName}/migrateBackups"].post | ||
- $.paths["/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.NetApp/netAppAccounts/{accountName}/migrateBackups"].post | ||
- code: GetCollectionOnlyHasValueAndNextLink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
This API was approved in a PR in the private repository in https://github.com/Azure/azure-rest-api-specs-pr/pull/8331
There where dealys in getting it out but we are now bringing it now in prevew.
Fixing this response model according to the rule would requre changing the reponse which is something the team is trying to avoid this late in the game.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rkmanda Would modeling this something like /latestBackupStatus/current
suffice ?
Will we need a collection GET as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test, removed suppression added default name, GetCollectionOnlyHasValueAndNextLink
does not show.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rkmanda Since the service has been deployed would it be possible to keep this as is for this preview api-version and fix in the next preview version? Is that a posibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to remove this from this version and introduce correctly modeled in the next. The preview clients have a workaround for this. This would give the team the time to patch the service side code needed.
Automatic PR validation restarted. This comment will be populated with next steps to merge this PR once validation is completed. Please wait ⌛. |
Automatic PR validation restarted. This comment will be populated with next steps to merge this PR once validation is completed. Please wait ⌛. |
/azp run |
Automatic PR validation started. This comment will be populated with next steps to merge this PR once validation is completed. Please wait ⌛. |
Azure Pipelines successfully started running 2 pipeline(s). |
Swagger pipeline restarted successfully, please wait for status update in this comment. |
ARM (Control Plane) API Specification Update Pull Request
Purpose of this PR
What's the purpose of this PR? Check all that apply. This is mandatory!
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:
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can request review from an ARM API Review board.
ARM API changes review
ARMReview
label.ARMReview
label, if appropriate.If this happens, proceed according to guidance given in GitHub comments also added by the automation.
Breaking change review
If you have any breaking changes as defined in the Breaking Change Policy,
follow the process outlined in the High-level Breaking Change Process doc.
Getting help
and https://aka.ms/ci-fix.