RecoveryServices Identities and Usages APIs by dragonfly91 · Pull Request #920 · Azure/azure-rest-api-specs

dragonfly91 · 2017-02-09T09:29:35Z

Adding registeredIdentities, vault extended info APIs and vault usages API. Also making this a composite client.

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

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

amarzavery · 2017-02-10T00:33:32Z

@dragonfly91 - Hello, thanks for sending the PR.

Can you please provide (1 minimum [just the required properties/parameters for making a request] and 1 maximum [full blown request with all the required and optional properties]) examples for request and response using the extension "x-ms-examples"? This will

help us validate the the swagger spec using the examples and catch errors early on and
it will also form the basis for customer facing REST API documentation on docs.microsoft.com website.

You can find more info about the extension and an example swagger spec over here.

dsgouda · 2017-02-10T23:09:08Z

This is not used anywhere in document, just wondering if it is required

This was output by Swashbuckle tool by default and we can't remove it while document generation. Our previous PR also contained this object but was merged still.

Remove it after document generation. Note that generating using Swashbuckle is supposed to be a one time thing, so editing this out should not be an issue. We will not merge (and should not have merged) it like that. SDK's providing their own (unused) Object type are super weird.

In our team, we have a deeply integrated Swashbuckle tier with which we are aiming to generate the specs with a quality that is review ready. So it is not one time for us. Anyway, we can write a post-script to remove the Object definition manually. Will update the PR by addressing this comment.

dsgouda · 2017-02-10T23:10:20Z

Consider x-ms-client-flatten to avoid nested properties

This is a polymorphic response type and we observed previously that this structure needs to be preserved for polymorphism to work in the intended manner.

dsgouda · 2017-02-10T23:12:12Z

Consider setting format to byte (for base64 as per swagger specs)

dsgouda · 2017-02-10T23:13:43Z

type:string and format:byte for base64 encoding

dsgouda · 2017-02-10T23:14:13Z

dsgouda · 2017-02-10T23:16:18Z

discriminator should be set to required

This is addressed.

dsgouda · 2017-02-10T23:18:10Z

Consider setting a min/max length and a regex all params like these

Is this a new requirement? This was not suggested in the previous PRs. Setting such constraints for global params in each Swagger spec - isn't this a bit redundant; where every team has to add the exact same settings?

Disregard this comment

@dragonfly91 Our set of requirements is constantly in motion, "previous PRs" always let stuff pass that won't pass now, even if it's just because a reviewer missed something.
Regarding min/max length or regex patterns: If there is a well defined and strictly enforced (by your service) contract that is most likely not going to change soon, it makes sense to specify it here. For example: Thinking of vaultName, if your service rejects empty string, please do provide a min length of 1. The client side validation will not only fail faster but also provide a better exception!

dsgouda · 2017-02-10T23:29:33Z

Doesn't look like this is used anywhere

This is output by Swashbuckle by default and we were not able to get rid of it. Can this be ignored?

@amarzavery do you have an opinion here?

dragonfly91 · 2017-02-20T12:52:10Z

@amarzavery, is providing x-ms-examples a blocker for the review to pass?

We use Swashbuckle to auto generate the swagger specs and this new requirement is not yet supported in the tool. So we need to write extra filters and tooling to auto generate the example files. We are in the middle of a release cycle right now and this would add extra cost; we would really like to take this up in the upcoming PR. Is it fine?

dsgouda · 2017-02-24T00:55:15Z

The correct convention to name operations here would be either "usages_list" or "usages_listbyvaults"

Vault usage represents usage per vault. The guideline states that the first half of Operation ID should be noun; VaultUsage is a compound noun. What is the issue with this name?

We are updating some of these guidelines. "Usages" should be the noun here, we plan to discourage such compound nouns moving forward. When fetching collections, the list operation should be named as either "CollectionItem_List" or "CollectionItem_ListByParent" where parent is the immediate preceding type in the url, which is "vaults" in this case

dsgouda · 2017-02-24T00:58:25Z

'PUT' operations should be named as "_create" or "_update"

dsgouda · 2017-02-24T01:04:02Z

An Operations API must be exposed which lists all the operations available

dsgouda · 2017-02-24T01:05:13Z

An Operations API must be exposed which lists all the operations available

dsgouda · 2017-02-24T01:07:34Z

ResourceCertificateDetailsResource is a tracked resource which must have a "GET" operation and a "PATCH" operation (which supports at least updating of tags)

We don't have GET and PATCH APIs supported in our service as of now. Is this a blocking requirement? We will open a bug on our backend service to support these APIs and once they are in production, we can add the Swagger descriptions for those APIs. Will this work?

For newer swagger specs we're considering this as a blocking requirement. @ravbhatnagar FYI

Agreed, GET/PATCH are required. @dragonfly91 - Have these APIs gone through the ARM/Azure API review ?

dsgouda · 2017-02-24T01:09:20Z

An Operations API must be exposed which lists all the operations available

I understand that the requirement seems to be to support one ListOperations API per swagger spec. We do have Operation_List APIs for each doc, but we have not yet exposed some of the APIs in Swagger yet. So, is it okay if the Operation_List API returns more APIs than that are defined in the spec?

After some offline discussions it seems exposing the operations API as-is would be the right thing to do.
Alternatively:

You could expose all the right APIs in the swagger documents for completeness OR

You could return only those APIs that are currently in swagger from the operations API
@ravbhatnagar could you chime in here?

Yep, exposing all operations through the /operations API is required. Please fix!

dsgouda · 2017-02-24T01:11:05Z

@amarzavery do you have an opinion here?

dsgouda · 2017-02-24T01:13:17Z

Apart from the latest validation rules' violations, PR seems fine, @olydis / @amarzavery would appreciate if you took a cursory look

olydis · 2017-02-24T17:18:39Z

Looks like a result-only data type. If so, please mark all properties as readonly. Same applies to MonitoringSummary and JobsSummary. If primitive member are guaranteed to exist in server responses (if they are "required" to be sent), mark them as x-nullable: false to provide a better user experience for the resulting .Net SDK.

olydis · 2017-02-24T17:23:21Z

uhm what is this? not used, so please remove

olydis · 2017-02-24T17:27:52Z

Is the order of parameters intended? I have no clear expectations regarding this service, but it seems odd that vaultName is named prior to resourceGroupName, especially since this is the reverse order of how stuff appears in the path! Does this generate an intuitive method signature? (this comment may apply to other methods)

olydis · 2017-02-24T17:29:38Z

is this an intuitive name? e.g. are both the Resource prefix and suffix crucial?

olydis · 2017-02-24T17:32:43Z

hmmm, a property named "properties" that returns something of type "...Details". @amarzavery Is this convention regarding extending Resource types? Otherwise this mismatch feels a little counter-intuitive.

olydis · 2017-02-24T17:35:23Z

duplicate description (or not really specific to this usage). The "encodedness" seems to be a property of the type and not this usage, so simply say that in the type's description and remove this one.

olydis · 2017-02-24T17:37:09Z

is properties required or is your service happy about empty requests? Mark the property as required otherwise.

olydis · 2017-02-24T17:38:03Z

This looks like a request data type, but none of the properties are marked as required. What does your service do if some of those are missing? I guess at least certificate is required, so please mark it accordingly.

olydis · 2017-02-24T17:45:52Z

Remove it after document generation. Note that generating using Swashbuckle is supposed to be a one time thing, so editing this out should not be an issue. We will not merge (and should not have merged) it like that. SDK's providing their own (unused) Object type are super weird.

olydis · 2017-02-24T17:51:09Z

@dragonfly91 Our set of requirements is constantly in motion, "previous PRs" always let stuff pass that won't pass now, even if it's just because a reviewer missed something.
Regarding min/max length or regex patterns: If there is a well defined and strictly enforced (by your service) contract that is most likely not going to change soon, it makes sense to specify it here. For example: Thinking of vaultName, if your service rejects empty string, please do provide a min length of 1. The client side validation will not only fail faster but also provide a better exception!

dsgouda · 2017-03-07T22:33:27Z

id, type and name properties for Resource should be marked as read-only

dsgouda · 2017-03-07T22:37:16Z

Paths should follow this format Subscriptions/{subscriptionId}/ResourceGroups/{resourceGroupName}/providers/namespace/typename1/{typename1type}/typename2/{typename2type}/operationname

Any updates?

Our service contains this API which is in production and we can't afford to change this at time point. Can't take this change now.

Sure, that works, please mark it as a future requirement since this might become a blocker moving forward

dsgouda · 2017-03-07T22:40:41Z

patch operations should be named as '_update'

dsgouda · 2017-03-07T22:42:11Z

nit: Is this an operation that could take a long time (>30 seconds) if so we might need a 202 response and x-ms-long-running-operation set to true. This applies to the delete operation as well

Any updates here?

This API doesn't take more than 30 seconds; we haven't modeled it as a long running operation in our back-end. So, this comment doesn't apply to us.

dsgouda · 2017-03-07T22:44:38Z

Vault is a tracked resource, please expose a PATCH operation for it

dsgouda · 2017-03-07T22:46:04Z

Mark resource properties "id, name, tag" as readonly

dsgouda · 2017-03-07T22:46:33Z

Per @olydis comments remove this definition even if it is autogenerated by swashbuckle

olydis · 2017-03-08T02:19:04Z

Note that most of our CI jobs are currently failing. As far as I can tell it's due to compositeRecoveryServicesClient.json being invalid JSON (trailing comma in array).

dsgouda · 2017-03-17T21:13:23Z

+        "produces": [
+          "application/json"
+        ],
+        "parameters": [


nit: maintain consistency, either place SubscriptionId and ApiVersion together at the beginning of the parameters section or at the end

dsgouda · 2017-03-17T21:13:57Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/ApiVersion"


nit: same comment as above

dsgouda · 2017-03-17T21:14:41Z

+            "$ref": "#/parameters/ApiVersion"
+          },
+          {
+            "$ref": "#/parameters/VaultName"


Definitely maintain parameter ordering here as it appears in the path, that makes for better user experience

dsgouda · 2017-03-17T21:18:20Z

+      "description": "Summary of the replication monitoring data for this vault.",
+      "type": "object",
+      "properties": {
+        "unHealthyVmCount": {


nit: typo unhealthyVmCount

Can't take this comment, as this involves changing the service contract.

dsgouda · 2017-03-17T21:19:23Z

+        }
+      }
+    },
+    "Object": {


Per earlier discussions please remove these Object definitions even if swashbuckle autogenerated these

dsgouda · 2017-03-17T21:31:29Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/ApiVersion"


nit: Parameter ordering

dsgouda · 2017-03-17T21:32:01Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/ApiVersion"


Fix parameter ordering

dsgouda · 2017-03-17T21:34:27Z

+          "type": "string",
+          "readOnly": true
+        },
+        "blobDuration": {


Is this a string in date-time format, please confirm

Yes, this is a string in date-time format

Please set the format accordingly

dsgouda · 2017-03-17T21:37:40Z

+          "readOnly": true
+        },
+        "Properties": {
+          "$ref": "#/definitions/ClientDiscoveryForProperties",


ClientDiscoveryForProperties does not sound right semantically. Did you mean ClientDiscoveryProperties

dsgouda · 2017-03-17T21:55:55Z

+      },
+      "x-ms-discriminator-value": "AzureActiveDirectory"
+    },
+    "ResourceCertificateAndACSDetails": {


Is this being used/allOfed anywhere? What is the discriminator field here given you are setting the x-ms-discriminator extension

This is a derived class.

dsgouda

A bunch of model definitions are being repeated across files, it's better if they are consolidated instead given we have a composite swagger here anyway
Also, if model definitions are not being used, please remove them

dsgouda · 2017-03-17T21:58:34Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/SubscriptionId"


nit: parameter ordering

dsgouda · 2017-03-17T22:03:01Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/SubscriptionId"


nit: parameter ordering

dsgouda · 2017-03-17T22:03:28Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/SubscriptionId"


nit:parameter ordering

dsgouda · 2017-03-17T22:05:00Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/SubscriptionId"


nit: parameter ordering

dsgouda · 2017-03-17T22:05:24Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/SubscriptionId"


nit:parameter ordering

dsgouda · 2017-03-17T22:07:07Z

+        }
+      }
+    },
+    "ClientDiscoveryForProperties": {


This is being defined twice, can we define it in a single definitions section instead and refer accordingly, this applies to all model definitions being repeated here

Do you mean to say that this is repeated across multiple JSONs? Is there a way to share definitions across swagger json files?

Yes the model definitions are repeated across JSONs. If you believe these definitions are going to be the same for these JSONs it makes sense to put them in a common spec and refer it from there. (Given that the JSONs here belong to the same namespace i.e. Microsoft.RecoveryServices I would believe that to be the case)
Yes, you can certainly share definitions across JSONs. Here is an example

azure-rest-api-specs/arm-network/2016-12-01/swagger/vmssNetworkInterface.json

Line 90 in dcd97d4

"$ref": "./networkInterface.json#/definitions/NetworkInterfaceListResult"

dsgouda · 2017-03-17T22:10:48Z

+      },
+      "x-ms-azure-resource": true
+    },
+    "NonTrackedResource": {


NonTrackedResource cannot have the location property. Also if you wish to have tracked and proxy (non-tracked) resources do the following:

Define a base class called Resource with id, name, tag readonly properties

Define a tracked resource with location as required property that allOfs on Resource

Define a proxy resource (non tracked resource) that allOfs on Resource and has any additional properties you wish to add

dsgouda

Requesting some Minor changes and updates on some previous comments, looks good otherwise

dsgouda · 2017-03-21T18:39:56Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/SubscriptionId"


nit: Please maintain consistency with parameter ordering, either put SubscriptionId and ApiVersion together at the beginning or at the end of the parameters section

dsgouda · 2017-03-21T18:40:10Z

+        ],
+        "parameters": [
+          {
+            "$ref": "#/parameters/SubscriptionId"


nit: parameter ordering

dsgouda · 2017-03-21T19:01:27Z

+          "application/json"
+        ],
+        "parameters": [
+          {


nit: parameter ordering

dsgouda · 2017-03-21T19:01:49Z

+          "application/json"
+        ],
+        "parameters": [
+          {


nit: parameter ordering

dsgouda · 2017-03-21T19:08:27Z

+        }
+      }
+    },
+    "ResourceCertificateAndAADDetails": {


Unless I'm mistaken there are duplicate properties in ResourceCertificateAndAADDetails and ResourceCertificateAndACSDetails, why not move them in ResourceCertificateDetails

dsgouda · 2017-03-21T19:15:10Z

+          "application/json"
+        ],
+        "parameters": [
+          {


nit parameter ordering

dsgouda · 2017-03-21T19:15:50Z

Any updates?

dsgouda · 2017-03-21T19:18:02Z

+        }
+      }
+    },
+    "Resource": {


Please set the extension x-ms-azure-resource to true

We don't want to model Resource as a tracked resource. Only the "TrackedResource" should be an ARM resource and Vault derives from TrackedResource, which derives from Resource.

You can set move the x-ms-azure-resource extension from "TrackedResource" to "Resource". The extension does not determine if a resource is "TrackedResource" or not, the required property location does that

dsgouda · 2017-03-21T19:19:05Z

+            "$ref": "#/definitions/Vault"
+          }
+        },
+        "nextLink": {


mark this as readonly

dsgouda · 2017-03-21T19:19:55Z

+      }
+    },
+    "VaultExtendedInfo": {
+      "description": "Vault extended information.",


nit: This is a part of request and response objects, please ensure the right properties have been set to readonly

Planning to take this up in the next iteration

I don't think that is a good idea. Parameters that can be set only at the server side should be marked readonly accordingly. Not doing this would allow users to send incorrect request to the server and result in a bad developer experience.
@olydis thoughts?

@dragonfly91 @dsgouda Indeed, we even had unusable SDK methods in the past because of that very issue. It boils down to this: readonly not only improves the developer experience, but also causes properties to not be serialized by generated clients during requests. We had RPs that rejected POST/PUT requests containing properties that are readonly from the services perspective. I'm not sure about your RP, but please be aware of this problem and make sure - not only for the developer experience, but actual basic usability - that sending those properties makes sense for your service!

I've confirmed that none of the properties in this class are eligible for readOnly:true; all the properties can be set during Creation / Update.

Thanks for confirming this

dragonfly91 · 2017-03-21T23:27:03Z

I took care of the param ordering comments and the comment about restructuring the Resource Certificate. Planning to take up remaining comments in the next iteration.

dsgouda · 2017-03-21T23:35:45Z

@azuresdkci Test this please

dsgouda

Looks good to me

dsgouda · 2017-03-22T22:25:12Z

@sarangan12 Can you make required changes in the ruby sdk to unblock the failing CI builds

dsgouda · 2017-03-23T18:58:05Z

+    "description": "Composite Swagger for Recovery Services Client"
+  },
+  "documents": [
+    "./2015-11-10/swagger/replicationusages.json",


So, I missed this earlier. You cannot have different version references like so. What you want to do here is keep the 2015-11-10/swagger/replicationusages.json file as it is and create a new file 2016-06-01/swagger/replicationusages.json and have it referenced here.

dsgouda

Please address comment

Removed unwanted spec Updating composite json

dsgouda · 2017-03-27T16:48:34Z

Ruby builds are failing since we need to make changes in the Ruby sdk repo to reflect the files corresponding to this RP
@sarangan12 will be making these changes once we merge this PR

azuresdkci · 2017-03-27T16:48:35Z

Can one of the admins verify this patch?

AutorestCI · 2017-03-27T16:55:54Z

No modification for NodeJS

AutorestCI · 2017-03-27T16:57:04Z

No modification for Python

* MS.K8 ARM Spec * Updated readme.ruby.md * Removed azure-sdk-for-node

azurecla added the cla-not-required label Feb 9, 2017

azuresdkci assigned dsgouda Feb 9, 2017

amarzavery added the Recovery Services Backup label Feb 10, 2017

dsgouda reviewed Feb 10, 2017

View reviewed changes

dragonfly91 force-pushed the master branch 2 times, most recently from e76550c to f44da91 Compare February 22, 2017 08:09

dsgouda reviewed Feb 24, 2017

View reviewed changes

olydis reviewed Feb 24, 2017

View reviewed changes

dsgouda reviewed Mar 7, 2017

View reviewed changes

dragonfly91 force-pushed the master branch from 2eafa4a to 15ff4d3 Compare March 17, 2017 05:42

Took review feedback in Recovery Services

44cb567

dragonfly91 force-pushed the master branch from 7fa19c2 to 44cb567 Compare March 17, 2017 06:33

dsgouda suggested changes Mar 17, 2017

View reviewed changes

dsgouda reviewed Mar 17, 2017

View reviewed changes

Took review feedback

99ccbf2

dsgouda suggested changes Mar 21, 2017

View reviewed changes

param ordering and ResourceCert reordering

0fd7b8e

Azure Resource changes

b678d0c

dsgouda approved these changes Mar 22, 2017

View reviewed changes

sarangan12 mentioned this pull request Mar 23, 2017

Change Recovery Services swagger to composite swagger Azure/azure-sdk-for-ruby#729

Merged

dsgouda reviewed Mar 23, 2017

View reviewed changes

dsgouda suggested changes Mar 23, 2017

View reviewed changes

Replication usages moved to latest version

a194f33

Removed unwanted spec Updating composite json

dragonfly91 force-pushed the master branch from f92482b to a194f33 Compare March 24, 2017 21:39

dsgouda approved these changes Mar 27, 2017

View reviewed changes

dsgouda merged commit bd58a94 into Azure:master Mar 27, 2017

dragonfly91 mentioned this pull request Apr 4, 2017

RecoveryServicess on latest Swagger Azure/azure-sdk-for-net#3028

Merged

8 tasks

mccleanp pushed a commit that referenced this pull request Mar 23, 2022

MS.K8 ARM Spec (#920)

c9d5fef

* MS.K8 ARM Spec * Updated readme.ruby.md * Removed azure-sdk-for-node

Conversation

dragonfly91 commented Feb 9, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

amarzavery commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragonfly91 Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragonfly91 commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragonfly91 commented Feb 9, 2017 •

edited by azuresdkci

Loading

amarzavery commented Feb 10, 2017 •

edited

Loading

dragonfly91 Feb 22, 2017 •

edited

Loading

dragonfly91 commented Feb 20, 2017 •

edited

Loading