-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Swagger for canceling MI management operations #7924
Conversation
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
"modelAsString": true | ||
} | ||
}, | ||
"errorCode": { |
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.
is there min and max?
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.
I am not sure that I understand what you mean.
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.
Also can you add other people for ARM team, I don't have permission to do that. Jared is on vacation until the end of the year.
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Can one of the admins verify this patch? |
@yungezz can you please merge this pull request? |
hi @toki95 , this PR is adding new version, need ARM review and signoff. |
@yungezz what does that mean? Adam Krom has signed off and he is from ARM team since Jared if oof until end of the year. |
hi @akromm could you pls label the PR with "ARMSignedOff"? |
{ | ||
"swagger": "2.0", | ||
"info": { | ||
"version": "2019-06-01-preview", |
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.
For future PRs please copy the previous version's swagger spec in commit 1 before making changes in future commits. This makes it much easier to review.
"$ref": "#/parameters/ManagedInstanceNameParameter" | ||
}, | ||
{ | ||
"name": "operationId", |
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.
This should be "operationName". "ID" typically refers to the full resource ID and the list of operations gives the value you want the user to provider here in the "name" property
"application/json" | ||
], | ||
"paths": { | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/managedInstances/{managedInstanceName}/operations/{operationId}/cancel": { |
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.
You need a GET API for individual operations to be compliant with the RPC especially if you are now allowing people to cancel individual operations. A comparison is operations on a template deployment: https://docs.microsoft.com/en-us/rest/api/resources/deploymentoperations/get
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.
This API for cancel was done in a manner to be compliant with SQL DB and they don't have an API fro getting one particular operation.
Also can you please explain if there is a requirement to create a new API or if we can add not-required parameter in existing API for listing all operations-operationId which will allow customer to get one particular operation.
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.
Existing APIs not following the RPC shouldn't be a reason to continue that trend. Services within Azure rely on RPs following the RPC so they can work with resources in a generic way without needing to know the quirks of any specific API.
I have not seen an example in existing swagger specs where someone modeled a LIST and GET operation in the same operation via an optional name parameter. The response model would be different so I don't imagine that is a good idea.
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.
I don't think that we understood each other entirely. What I was thinking of doing is adding new key in existing API for getting all operations.
Current call of API:
https://management.azure.com/subscriptions/a8c9a924-06c0-4bde-9788-e7b1370969e1/resourceGroups/v-urmila/providers/Microsoft.Sql/managedInstances/v-urmila-mi-test/operations?api-version=2018-06-01-preview
which returns all operations to customer.
And adding a key operationId the same API can be called like this:
https://management.azure.com/subscriptions/a8c9a924-06c0-4bde-9788-e7b1370969e1/resourceGroups/v-urmila/providers/Microsoft.Sql/managedInstances/v-urmila-mi-test/operations/a64077e8-43e6-47b6-a6ef-9c2f40fac38b?api-version=2018-06-01-preview
pls address ARM reviewing comments. |
hi @toki95 pls address reviewing comments . |
hi @toki95 pls address reviewing comments . |
hi @toki95 any update? thanks. |
ping again. @toki95 |
Addressing comments
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines failed to run 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Added example for getting MI operation.
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Adding swagger for cancel and list of management operations on managed server.