-
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
Some APIs do not have terminal response definition - Added code to fix it. #814
Conversation
Hi @sarangan12, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
@@ -4561,6 +4681,14 @@ | |||
} | |||
}, | |||
"x-ms-azure-resource": true | |||
}, | |||
"GenericOkResponse": { |
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.
Can we have a better name for the model. This will be the return type for all the POST (action) operations?
How about OperationStatus
?
}, | ||
"GenericOkResponse": { | ||
"properties": { | ||
"status": { |
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 model also contains other properties like
- startTime
- endTime
- name
Why are they not modeled over here?
@sarangan12 - Can you please resolve the comments made in the PR? |
"description": "", | ||
"schema": { | ||
"$ref": "#/definitions/GenericOkResponse" | ||
} | ||
} | ||
}, | ||
"x-ms-long-running-operation": 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.
As per the swagger rule M2005, seems like they are describing that for delete
operation valid terminal success status codes are "204 or 200 or both for delete".
Either the rule is not complete or something i am misunderstanding.
@amarzavery / @sarangan12 Could one of you please clarify?
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.
@amarzavery Any thoughts on this one?
@amarzavery Can you please review the updated swagger? |
Snapshots attached to indicate the output as a result of this change. |
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger
Related to issue: Azure/azure-sdk-for-ruby#566
@amarzavery @veronicagg @vishrutshah @salameer Please review.