-
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
[Not be be merged] Swagger for SiteRecovery #1016
Conversation
…into masterCurrent
…into masterCurrent
…into masterCurrent
@avneeshrai, |
@avneeshrai Please take a look at failures from our validation tool at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209296153#L963 |
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.
Avneesh, Veronica sent an email regarding suggestions and fixes required, kindly look into them and let us know if you have further questions
@veronicagg , @salameer I was on vacations. Would have a look at suggestions and fixes and get back. |
@avneeshrai I replied to an email from you last week, feel free to make updates and let us know if you have other questions, we can continue the discussion here. |
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.
Some additional comments.
"application/json" | ||
], | ||
"paths": { | ||
"/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{resourceNamespace}/{resourceType}/{resourceName}/replicationFabrics/{fabricName}/replicationvCenters/{vCenterName}/operationresults/{jobName}": { |
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.
should Subscriptions start with lowercase?
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.
resourceNamspace shouldn't be parameterized, there should be one provider per spec, recommendation is to split the swagger files.
"VCenters" | ||
], | ||
"summary": "Tracks the provider async operation.", | ||
"description": "TODO (avrai): Detailed Description.", |
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's a TODO for the description.
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.
We are still working on documentation and examples part. We will update the PR once they are ready.
], | ||
"responses": { | ||
"200": { | ||
"descrip |
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 location required?
}, | ||
{ | ||
"name": "addVCenterRequest", | ||
"in": "body", |
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 a reason for the body parameter not being the same model as the response?
M2017: The model definition for the body parameter and the response MUST be the same for a PUT operation. This ensures that the same entity will be reusable between GET and PUT. (from https://github.com/Azure/azure-rest-api-specs/blob/62c96876795e600de8369efe42edd3a70f494017/documentation/swagger-checklist.md)
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.
Our team had referred these links at the time of development of RestAPIS:
http://sharepoint/sites/AzureUX/Sparta/_layouts/15/WopiFrame.aspx?sourcedoc={CC855351-793A-4417-A536-AB992218AD55}&file=Resource%20Provider%20API%20v2.docx&action=default
and
http://sharepoint/sites/azure-arc/_layouts/15/WopiFrame.aspx?sourcedoc={e3be54e1-c7a7-4e08-8c12-32670989e599}&action=view&Source=http%3A%2F%2Fsharepoint%2Fsites%2Fazure-arc%2FSitePages%2FREST%2520Guidelines%2Easpx&DefaultItemOpen=1
Also, our RESTAPI model has been reviewed multiple times by ARM team and we never this feedback.
Having said that, at this point it would be not possible for us to incorporate this request of having same model definition for GET and PUT looking at the impact and efforts needed.
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.
SiteRecovery's REST API doc which was approved by ARM for reference:
https://microsoft.sharepoint.com/teams/cdm-idc/_layouts/15/WopiFrame.aspx?sourcedoc={C9F91552-0255-44B5-A57D-106275A37D14}&file=Azure%20Site%20Recovery%20CSMV2%20API%20Spec.docx&action=default
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.
Lets discuss if you more clarification is needed.
@veronicagg I have replied to your mail on the mail thread. To continue discussion here copy pasting the content: #1. Instead of parameterizing the resourceNamespace, could you create 2 swagger specs where the value is instantiated in each? You can cross reference model definitions and parameters as needed. I’ve been told you would be deprecating one of them soon, if so, then this way you can deprecate one of the swaggers, instead of introducing breaking changes in the non-deprecated one. If this doesn’t work for you, please let us know why. #2. Is the name “ReplicationVCenters_ListByReplicationFabrics” not good in this case? #3. Could you paste the errors you’re getting? The ARMResourcePropertyBag rule is returning some false positives, which we’re fixing at the moment, but there may still be valid errors. This rule should be flagging duplicated properties, so if you have defined properties that are the same as what your Resource contains, then you should rename those. #4. TrackedResourceValidation is being updated at the moment to be more accurate. Please review which one of your resources is a Tracked Resource and make sure: |
#1. I don't know how other RPs deal with testing environments. Regarding 2 providers in the same file, we should check that generated SDKs would actually be produced correctly, and how they would look, I don't think we have other examples of this. Per swagger spec: "Declares the value of the parameter that the server will use if none is provided, for example a "count" to control the number of results per page might default to 100 if not supplied by the client in the request. (Note: "default" has no meaning for required parameters.) " (from http://swagger.io/specification/#parameterObject). If it's a required parameter, it requires the customer to provide a value. @ravbhatnagar could provide your thoughts on this? how do other RPs deal with testing environments? |
…into masterCurrent
…sages (Examples, documentation are still to be worked on. Few RPC voilations would be taken with next service upgrade and for others we have exception)
Closing this PR. Would raise a fresh PR after taking all comments and closures with appropriate commits. |
Please ignore:
They would be corrected in final PR. Intent is to take early feedback.