[Not be be merged] Swagger for SiteRecovery #1016

avneeshrai · 2017-03-09T10:16:14Z

Please ignore:

meaningless commit names.
Documentation fields.
They would be corrected in final PR. Intent is to take early feedback.

…into masterCurrent

msftclas · 2017-03-09T10:16:17Z

@avneeshrai,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

veronicagg · 2017-03-10T03:05:33Z

@avneeshrai Please take a look at failures from our validation tool at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209296153#L963
Additionally, examples should be included for the operations, please see: https://github.com/Azure/azure-rest-api-specs-pr/blob/b9ae70798c1b8c2efef4c14bc0120e1759b6ba23/documentation/swagger-checklist.md#validation-tools-for-swagger-checklist
Thanks!

salameer

Avneesh, Veronica sent an email regarding suggestions and fixes required, kindly look into them and let us know if you have further questions

avneeshrai · 2017-03-20T06:53:00Z

@veronicagg , @salameer I was on vacations. Would have a look at suggestions and fixes and get back.

veronicagg · 2017-03-20T17:20:14Z

@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.

veronicagg

Some additional comments.

veronicagg · 2017-03-11T01:40:44Z

arm-siterecovery/2015-11-10/swagger/service.json

+    "application/json"
+  ],
+  "paths": {
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{resourceNamespace}/{resourceType}/{resourceName}/replicationFabrics/{fabricName}/replicationvCenters/{vCenterName}/operationresults/{jobName}": {


should Subscriptions start with lowercase?

resourceNamspace shouldn't be parameterized, there should be one provider per spec, recommendation is to split the swagger files.

veronicagg · 2017-03-20T17:22:34Z

arm-siterecovery/2015-11-10/swagger/service.json

+          "VCenters"
+        ],
+        "summary": "Tracks the provider async operation.",
+        "description": "TODO (avrai): Detailed Description.",


there's a TODO for the description.

We are still working on documentation and examples part. We will update the PR once they are ready.

veronicagg · 2017-03-20T17:26:16Z

arm-siterecovery/2015-11-10/swagger/service.json

+        ],
+        "responses": {
+          "200": {
+            "descrip


is location required?

veronicagg · 2017-03-21T00:59:50Z

arm-siterecovery/2015-11-10/swagger/service.json

+          },
+          {
+            "name": "addVCenterRequest",
+            "in": "body",


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)

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.

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

Lets discuss if you more clarification is needed.

avneeshrai · 2017-03-21T18:34:31Z

@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.
[AR] It would still not help us pointing to internal environments for testing (we have test dedicated ProviderNameSpaces).
Can you please help me understand what issues we see with default value approach as proposed earlier in the current mail thread?
As it addresses all our pain points:
• Amar’s concern was SDK customers should not be required to know or set provider namespace\type. With this customer doesn’t need to know or set any value explicitly. By default, it hits the intended namespace and type (Microsoft.RecoveryServices).
• It solves the issue of pointing to internal environments for our testing (by overriding the default values).
• We can also provide temporary support that we need for Microsoft.SiteRecovery type with PowerShell by explicitly setting values of provider namespace and type.
• No extra cost of maintaining and understanding two swagger specs at three layers (rest-api-spec, sdk-for-net and azure-powershell).
Just to mention, we would be publishing swagger in preview mode. And removing support for Microsoft.SiteRecovery later would require no change in swagger spec(if we use default value approach). We neither have nor we will have any customer using SDK for Microsoft.SiteRecovery. We just need to support it temporarily through PowerShell.
Please mention if you need more clarity, we can meet over skype to discuss it further.

#2. Is the name “ReplicationVCenters_ListByReplicationFabrics” not good in this case?
[AR] We can go ahead with this. Just that we wanted to avoid repeated usage of word ‘replication’.

#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.
[AR] An instance for example:
Path: service.json#/definitions
ERROR: Top level property names should not be repeated inside the properties bag. Properties [properties] conflict with ARM top level properties. Please rename
these.
It doesn’t clearly call out which property has problem.
Moreover, internal properties can have field names which are in Resource (Say for example, Host is the resource which will have id and a host can have Disks. Each Disk in turn will have id)? Please correct if my expectation is wrong.

#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:
• There’s a get operation which returns the tracked resource.
• If the tracked resource is a parent resource:
o There must be a get operation with id _ListByResourceGroup which returns the tracked resource
o There must be a get operation with id _List|_ListBySubscriptionId|_ListBySubscription|*_ListBySubscriptions which returns the tracked resource
• If the tracked resource is a child resource:
o There must be an operation with id *_ListBy[ParentResourceName] which returns the tracked resource
[AR] None of our resources are tracked resources.

veronicagg · 2017-03-22T00:29:58Z

#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?
#2. I see, ok, the suggested naming is to be consistent across other operations, if you think the name is confusing from a customer perspective, then let me know and we could make an exception.
#3. The error you pasted is a false positive, we’re fixing at the moment, we should not be flagging “properties” property. The examples you mentioned are correct, and having those properties at nested levels is fine.
#4. Thanks for confirming.

…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)

avneeshrai · 2017-04-05T18:46:18Z

Closing this PR. Would raise a fresh PR after taking all comments and closures with appropriate commits.

avneeshraiusit added 14 commits October 6, 2016 19:28

Adding test swagger for SiteRecovery service

638482d

temp

b575d8a

ODATA

d986d94

yet new

b495ad0

update me plzz

b8dec81

swag

f5fcb5c

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs …

41d2835

…into masterCurrent

sad

78119f6

asdas

657cb95

API version rename

9e0fce9

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs …

8fa81ce

…into masterCurrent

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs …

dfd81af

…into masterCurrent

asdsddasd

c424bb5

asdas

c90c1ff

msftclas added the cla-not-required label Mar 9, 2017

azuresdkci assigned veronicagg Mar 9, 2017

salameer requested changes Mar 17, 2017

View reviewed changes

veronicagg suggested changes Mar 21, 2017

View reviewed changes

avneeshraiusit added 2 commits March 23, 2017 18:36

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs …

cf66ef7

…into masterCurrent

Correcting the version and taking some review comments and linter mes…

c91ce15

…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)

avneeshrai closed this Apr 5, 2017

avneeshrai mentioned this pull request May 9, 2017

Adding swagger spec for RecoveryServices.SiteRecovery service. #1209

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Not be be merged] Swagger for SiteRecovery #1016

[Not be be merged] Swagger for SiteRecovery #1016

avneeshrai commented Mar 9, 2017 •

edited by azuresdkci

Loading

msftclas commented Mar 9, 2017

veronicagg commented Mar 10, 2017

salameer left a comment

avneeshrai commented Mar 20, 2017

veronicagg commented Mar 20, 2017

veronicagg left a comment

veronicagg Mar 11, 2017

veronicagg Mar 20, 2017

veronicagg Mar 20, 2017

avneeshrai Mar 21, 2017

veronicagg Mar 20, 2017

veronicagg Mar 21, 2017

avneeshrai Mar 23, 2017

avneeshrai Mar 23, 2017

avneeshrai Mar 23, 2017

avneeshrai commented Mar 21, 2017

veronicagg commented Mar 22, 2017

avneeshrai commented Apr 5, 2017

[Not be be merged] Swagger for SiteRecovery #1016

[Not be be merged] Swagger for SiteRecovery #1016

Conversation

avneeshrai commented Mar 9, 2017 • edited by azuresdkci Loading

msftclas commented Mar 9, 2017

veronicagg commented Mar 10, 2017

salameer left a comment

Choose a reason for hiding this comment

avneeshrai commented Mar 20, 2017

veronicagg commented Mar 20, 2017

veronicagg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avneeshrai commented Mar 21, 2017

veronicagg commented Mar 22, 2017

avneeshrai commented Apr 5, 2017

avneeshrai commented Mar 9, 2017 •

edited by azuresdkci

Loading