Adding swagger changes for Virtual Network Rules #1095

dnayantara · 2017-04-05T00:56:57Z

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.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

msftclas · 2017-04-05T00:57:01Z

@dnayantara,
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

nathannfan

Looks good! A few minor changes for it to work for SDK, but it's close.

nathannfan · 2017-04-05T01:03:47Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+  "info": {
+    "version": "2015-05-01-preview",
+    "title": "SqlManagementClient",
+    "description": "The Database management API provides a RESTful set of web APIs that interact with Database services to manage your databases. The API enables users to create, retrieve, update, and delete databases, servers, and other entities."


Could you update description to something more specific to VNetRules?

Sure. Makes sense

nathannfan · 2017-04-05T01:04:52Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+          "VnetFirewallRule"
+        ],
+        "description": "Returns information about a Vnet Firewall Rule.",
+        "operationId": "VnetFirewallRule_Get",


VnetFirewallRules

Sorry, I should clarify. VnetFirewallRules_Get - we've been going plural_operation for operation id

nathannfan · 2017-04-05T01:23:27Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+            }
+          }
+        },
+        "x-ms-long-running-operation": true


Do you return 202? If so, I think you'll need to add
"202": {
"description": "Accepted"
}

to responses. No need to include the response body for 202 that in the swagger or examples though, just the response code with an empty body.

I'm assuming this operation follows azure async pattern, which returns 202 initially?

Yes it does. I will add that.

nathannfan · 2017-04-05T01:24:20Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+    }
+  },
+  "definitions": {
+    "OperationListResult": {


Are the Operations definitions being used?

I dont think so. I didnt think we would have to do anything separately for the operations

You're right, we only need them in one file per API version. You can remove all the Operations definitions then.

This should also clear up one of the validation errors

nathannfan · 2017-04-05T02:55:12Z

@dnayantara Looks like the validation failures are because of parameter description mismatches. Copying the parameters from https://github.com/Azure/azure-rest-api-specs/blob/master/arm-sql/2015-05-01-preview/swagger/blobAuditingPolicies.json should fix it.

dsgouda

Requesting minor changes

dsgouda · 2017-04-05T21:04:01Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+          "Gets the virtual network firewall rule": { "$ref": "../examples/VirtualNetworkRulesGet.json" }
+        },
+        "parameters": [
+          {


nit: Maintain parameter ordering, place SubscriptionId and ApiVersion together at the beginning or the end of the parameters section

dsgouda · 2017-04-05T21:05:15Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+          "Create or update a virtual network firewall rule": { "$ref": "../examples/VirtualNetworkRulesCreateOrUpdate.json" }
+        },
+        "parameters": [
+          {


nit: parameter ordering

dsgouda · 2017-04-05T21:05:39Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        "x-ms-examples": {
+          "Delete a virtual network firewall rule": { "$ref": "../examples/VirtualNetworkRulesDelete.json" }
+        },
+        "parameters": [


nit: parameter ordering

dsgouda · 2017-04-05T21:06:01Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+          "Lists the virtual network firewall rules": { "$ref": "../examples/VirtualNetworkRulesList.json" }
+        },
+        "parameters": [
+          {


nit: parameter ordering

dsgouda · 2017-04-05T21:08:58Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        "properties": {
+          "$ref": "#/definitions/VnetFirewallRuleResourceProperties"
+        },
+        "location": {


Looks like a Resource, there are a couple of options here either reference this model or redefine the same structure here and do an allOf. (I'd recommend not redefining simply since its duplication and overhead)

Yes, please copy Resource+TrackedResource/ProxyResource from other SQL files and use allOf to include TrackedResource/ProxyResource as appropriate.

dsgouda · 2017-04-05T21:09:44Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+      "description": "Properties of a vnet firewall rule resource.",
+      "type": "object",
+      "properties": {
+        "virtualNetworkSubnetId": {


Should this be readonly? Or do you allow this as a part of the request object?

This is allowed as part of the request object. I dont think this is readonly

dsgouda · 2017-04-05T21:10:02Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+          }
+        },
+        "nextLink": {
+          "type": "string"


set this to readonly

jaredmoo · 2017-04-05T02:56:22Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+            "$ref": "#/parameters/ServerNameParameter"
+          },
+          {
+            "name": "vnetFirewallRuleName",


this should be name: parameters

name:parameters makes sense for a body parameter. This is correct

jaredmoo · 2017-04-05T21:41:03Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+          "VnetFirewallRule"
+        ],
+        "description": "Returns information about Vnet Firewall Rules.",
+        "operationId": "VnetFirewallRules_List",


should be VnetFirewallRules_ListByServer

jaredmoo · 2017-04-05T21:41:57Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        "properties": {
+          "$ref": "#/definitions/VnetFirewallRuleResourceProperties"
+        },
+        "location": {


Yes, please copy Resource+TrackedResource/ProxyResource from other SQL files and use allOf to include TrackedResource/ProxyResource as appropriate.

jaredmoo · 2017-04-05T21:42:17Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+      "type": "object",
+      "properties": {
+        "properties": {
+          "$ref": "#/definitions/VnetFirewallRuleResourceProperties"


Add x-ms-flatten:true

jaredmoo · 2017-04-05T21:42:37Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        }
+      }
+    },
+    "VnetFirewallRuleList": {


VnetFirewallRuleListResult

jaredmoo · 2017-04-05T21:43:20Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

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


Do you actually have paging? If no, remove this and add x-ms-paging: { nextLinkName: null } to the list operation.

jaredmoo · 2017-04-05T22:59:07Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        },
+        "type": {
+          "type": "string"
+        }


Is this a proxy resource? If so, remove location, id, name, and type from this definition and use allOf, e.g. https://github.com/Azure/azure-rest-api-specs/blob/master/arm-sql/2014-04-01/swagger/sql.core.json#L1627

jaredmoo · 2017-04-05T22:59:31Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        }
+      }
+    },
+    "VnetFirewallRuleResourceProperties": {


Should be named VnetFirewallRuleProperties

dsgouda · 2017-04-05T23:00:40Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+       "x-ms-paging": { 
+         "nextLinkName": "null" 
+         },
+        "parameters": [


Formatting looks a little off

dsgouda · 2017-04-05T23:04:17Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+    "VnetFirewallRule": {
+      "description": "Vnet Firewall Rule Resource.",
+      "type": "object",
+      "properties": {


What @jaredmoo said

jaredmoo · 2017-04-05T23:54:21Z

FYI the ListByOperationsValidation linter error regarding ListByServer should be ignored. See discussion with @veronicagg at #1005 (comment)

dsgouda · 2017-04-05T23:59:18Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        }
+      ]
+    },
+    "VnetFirewallRule": {


nit: What I implied was cross-referencing using definitions that already exist under your RP like so
https://github.com/Azure/azure-rest-api-specs/blob/master/arm-recoveryservices/2016-06-01/swagger/replicationusages.json#L46
This is not a requirement but reduces duplication

Not making this change as @jaredmoo said in the email, cross references don't work.

Replied on the email thread. However, won't block on this.

jaredmoo · 2017-04-05T23:55:44Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+      "in": "path",
+      "description": "The name of the Virtual Network Firewall Rule.",
+      "required": true,
+      "type": "string"


needs "x-ms-parameter-location": "method"

jaredmoo · 2017-04-05T23:57:41Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        "tags": [
+          "VnetFirewallRule"
+        ],
+        "description": "Returns information about a Vnet Firewall Rule.",


nit: can you use "virtual network firewall rule" (no capitalization) instead of "Vnet Firewall Rule" throughout this file? That will make Azure CLI commands look better when you get to it.

Ill change it to virtual network rules since thats what we are calling it to the customers.

jaredmoo · 2017-04-05T23:58:13Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        "tags": [
+          "VnetFirewallRule"
+        ],
+        "description": "Creates a new Vnet Firewall Rule or updates an existing Vnet Firewall Rule.",


Creates or updates a virtual network firewall rule

dsgouda · 2017-04-06T00:12:47Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+      "properties": {
+        "properties": {
+          "$ref": "#/definitions/VnetFirewallRuleProperties",
+          "x-ms-flatten": "true"


This should be x-ms-client-flatten

dsgouda · 2017-04-06T00:13:46Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+        "x-ms-examples": {
+          "Lists the virtual network firewall rules": { "$ref": "../examples/VirtualNetworkRulesList.json" }
+        },
+        "x-ms-paging": { 


This should be x-ms-pageable, please recheck extension names

dsgouda · 2017-04-06T21:09:02Z

@azuresdkci Test this please

dnayantara · 2017-04-06T21:20:05Z

I will add another iteration which has been directly generated from the source code soon.

dsgouda

Couple of minor comments and a suggestion. LGTM pending CI builds

dsgouda · 2017-04-06T23:22:20Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+      "type": "object",
+      "properties": {
+        "nextLink": {
+          "description": "Link to retrieve next page of results.",


Please mark this as readonly

dsgouda · 2017-04-06T23:23:51Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+            "$ref": "#/parameters/ServerNameParameter"
+          },
+          {
+            "name": "vnetFirewallRuleName",


name:parameters makes sense for a body parameter. This is correct

dsgouda · 2017-04-06T23:26:06Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

+            "in": "body",
+            "description": "The requested vnetFirewall Resource state.",
+            "required": true,
+            "schema": {


nit: Just want to point out that the VnetFirewallRule model has no required properties which means it could theoretically be an empty object, please verify that the service is fine with this.

No. the parameters should be required. I will change this.

dsgouda · 2017-04-07T18:02:59Z

arm-sql/2015-05-01-preview/swagger/virtualNetworkRules.json

@@ -250,6 +250,7 @@
      "x-ms-azure-resource": true
    },
    "ProxyResource": {


Did you plan to add required properties for VnetFirewallRule in this commit?

Lets just leave it like this for now. I may make changes later.

I'm fine with it as long as the service is OK with an empty body parameter

I am with the service team and we are ok with this for now. Please accept pull request if everything else looks good.

AutorestCI · 2017-04-07T18:51:43Z

No modification for NodeJS

AutorestCI · 2017-04-07T18:51:43Z

Azure/azure-sdk-for-python#1065

AutorestCI · 2017-04-07T18:52:00Z

Azure/azure-sdk-for-ruby#742

AutorestCI · 2017-04-07T18:52:43Z

No modification for Ruby

Adding swagger changes for Virtual Network Rules

4a8cc09

msftclas added the cla-not-required label Apr 5, 2017

azuresdkci assigned sergey-shandar Apr 5, 2017

nathannfan suggested changes Apr 5, 2017

View reviewed changes

Addressing comments

d006dc0

Updating parameter descriptions

885b8a2

salameer added the Reassign label Apr 5, 2017

salameer unassigned sergey-shandar Apr 5, 2017

azuresdkci removed the Reassign label Apr 5, 2017

azuresdkci assigned dsgouda Apr 5, 2017

dsgouda suggested changes Apr 5, 2017

View reviewed changes

jaredmoo suggested changes Apr 5, 2017

View reviewed changes

Addressing comments

0e79649

jaredmoo suggested changes Apr 5, 2017

View reviewed changes

dsgouda reviewed Apr 5, 2017

View reviewed changes

Addressing comments and making minor changes

943f90d

dsgouda reviewed Apr 5, 2017

View reviewed changes

jaredmoo suggested changes Apr 6, 2017

View reviewed changes

dsgouda reviewed Apr 6, 2017

View reviewed changes

Updating with auto-generated swagger document

2c41419

dsgouda approved these changes Apr 6, 2017

View reviewed changes

Addressing comments

4ff3b8e

dsgouda reviewed Apr 7, 2017

View reviewed changes

dsgouda merged commit 79c07cc into Azure:master Apr 7, 2017

dnayantara deleted the swaggerdocbranch branch April 7, 2017 18:48

AutorestCI mentioned this pull request Apr 7, 2017

[SQL] Adding swagger changes for Virtual Network Rules Azure/azure-sdk-for-python#1065

Closed

AutorestCI mentioned this pull request Apr 7, 2017

Automatic PR from RestAPI-PR1095 Azure/azure-sdk-for-ruby#742

Closed

jaredmoo mentioned this pull request Jun 7, 2017

SQL server AD identity and data encryption support Azure/azure-sdk-for-net#3334

Merged

8 tasks

Adding swagger changes for Virtual Network Rules #1095

Adding swagger changes for Virtual Network Rules #1095

Uh oh!

Conversation

dnayantara commented Apr 5, 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!

msftclas commented Apr 5, 2017

Uh oh!

nathannfan left a comment

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!

nathannfan commented Apr 5, 2017

Uh oh!

dsgouda left a comment

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!

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

dnayantara commented Apr 5, 2017 •

edited by azuresdkci

Loading