Add version 2016-10-10 for api management #1039

solankisamir · 2017-03-16T01:35:30Z

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.

anuchandy

Please provide x-ms-examples of each operation as well.

Reference: https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/x-ms-examples.md

anuchandy · 2017-03-17T20:57:40Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


Please define schema instead of using anonymous types.

I mean a separate schema for this collection in definitions like:

"PolicySnippetContractCollection": { "properties": { "value": { "type": "array", "items": { "$ref": "#/definitions/PolicySnippetContract" }, "description": "Result of the List list policy contracts operation." } }, "description": "The response of the list policy contracts operation." }

And refer it from here:

"schema": { "$ref": "#/definitions/PolicySnippetContractCollection" }

anuchandy · 2017-03-17T21:03:18Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


Avoid anonymous type as recommended for PolicySnippetContractCollection

anuchandy · 2017-03-17T21:51:05Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+        "tags": [
+          "GroupUsers"
+        ],
+        "operationId": "GroupUsers_Remove",


If we follow the naming convention then it should be named as GroupUsers_Delete

#fixed

In reply to: 106752535 [](ancestors = 106752535)

anuchandy · 2017-03-17T21:52:07Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "The user was successfully removed from the group.."
+          },
+          "405": {
+            "description": "Attempt was made to add a user to a built-in group. Built-in group membership is managed by the system.",


Is this error valid for delete scenario? the description says "Attempt was made to ADD a user to a built-in group"

anuchandy · 2017-03-17T21:56:57Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "required": false,
+            "type": "string",
+            "description": "| Field       | Supported operators    | Supported functions               |\n|-------------|------------------------|-----------------------------------|\n| id          | ge, le, eq, ne, gt, lt | substringof, startswith, endswith |\n| name        | ge, le, eq, ne, gt, lt | substringof, startswith, endswith |\n| description | ge, le, eq, ne, gt, lt | substringof, startswith, endswith |\n| serviceUrl  | ge, le, eq, ne, gt, lt | substringof, startswith, endswith |\n| path        | ge, le, eq, ne, gt, lt | substringof, startswith, endswith |"
+          },


Do you think it make sense to move $filter parameter to common section like other OData parameters (top and skip)?

No, these filters are different for each api. Each api has distinct properties, and different operators applied.

anuchandy · 2017-03-17T22:26:16Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Product was successfully created."
+          },
+          "204": {
+            "description": "Product already exists."


Does this means - i cannot use PUT to update product entry? The method name is CreateOrUpdate

The Description was incorrect. You can use PUT to updateproduct entity. #fixed

anuchandy · 2017-03-17T22:26:57Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+              "$ref": "#/definitions/ErrorBodyContract"
+            }
+          }
+        }


Is product PUT LRO? if so please model it so.

no PUT is not LRO.

anuchandy · 2017-03-17T22:31:09Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


"Subsctions" => "Subscriptions"

anuchandy · 2017-03-17T22:33:28Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


does count represents number of items in SubscriptionCollection. properties.value.items or total entries across all pages? Will be good to clarify that in the description

#fixed. It represents total entries across all pages.

anuchandy · 2017-03-17T22:36:52Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


"loggerid" is a parameter used ~4 times, so consider moving this to global parameter section and doing a $ref.

#fixed

In reply to: 106758104 [](ancestors = 106758104)

anuchandy · 2017-03-17T23:44:34Z

arm-apimanagement/2016-10-10/swagger/apimdeployment.json

+          "type": "string",
+          "description": "Base64 Encoded certificate."
+        },
+        "certificate_password": {


use camel casing here certificatePassword

We will take this as feedback and update it in next version of the API. This api-version is already being used, we are just updating the swagger for documentation purpose.

In reply to: 106763958 [](ancestors = 106763958)

anuchandy · 2017-03-17T23:50:35Z

arm-apimanagement/2016-10-10/swagger/apimdeployment.json

+    },
+    "ApiManagementServiceGetSsoTokenResult": {
+      "properties": {
+        "redirect_uri": {


use camel case redirectUri

We will take this as feedback and update it in next version of the API. This api-version is already being used, we are just updating the swagger for documentation purpose.

In reply to: 106764472 [](ancestors = 106764472)

anuchandy · 2017-03-17T23:51:15Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


use camel case rawJson

anuchandy · 2017-03-17T23:51:29Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


anuchandy · 2017-03-17T23:58:53Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


Try to avoid avoidAnonymous types, refer #1039 (comment)

anuchandy · 2017-03-18T00:09:30Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


If this is ISO8601 duration then specify "format": "duration"

anuchandy · 2017-03-18T00:29:47Z

arm-apimanagement/2016-10-10/swagger/apimdeployment.json

+      ],
+      "description": "Custom hostname configuration."
+    },    
+    "VirtualNetworkConfiguration": {


VirtualNetworkConfiguration- is this something user provides? If so, the values that can be set is subnetResourceId and location. Why are we asking location from user? can it be inferred from the subnetResourceId (assuming you have linked access enabled for network RP).

Is readonly property VirtualNetworkConfiguration::vnetid the resource id of vnet in which user provided subnet reside? the comment says it's a GUID.

Well, we support setting up API Management service in both Classic and ARM Vnets. For Classic VNET, we accept vnetId(GUID) and Location and for ARM VNet we only require subnetResourceId. For Classic, we are not integrated as linked resource.

anuchandy · 2017-03-18T00:36:18Z

arm-apimanagement/2016-10-10/swagger/apimdeployment.json

+      ],
+      "description": "API Management service resource SKU properties."
+    },
+    "ApiManagementServiceResource": {


Is ApiManagementServiceResource an ARM resource? if yes any reason not to define Resource ("x-ms-azure-resource") model and allOf from it?

anuchandy · 2017-03-18T00:42:27Z

arm-apimanagement/2016-10-10/swagger/apimanagement.json

+            "description": "Error response describing why the operation failed.",
+            "schema": {
+              "$ref": "#/definitions/ErrorBodyContract"
+


This should be Apis_get

solankisamir · 2017-03-18T23:57:21Z

Closed this review, as it took some files from the bad branch, which were not intended. Have incorporated the feedback into the new pull request.

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

fearthecowboy added the Reassign label Mar 17, 2017

azuresdkci removed the Reassign label Mar 17, 2017

azuresdkci assigned vishrutshah Mar 17, 2017

anuchandy reviewed Mar 17, 2017

View reviewed changes

anuchandy reviewed Mar 18, 2017

View reviewed changes

solankisamir closed this Mar 18, 2017

solankisamir force-pushed the master branch from 8c36a91 to d5d33d9 Compare March 18, 2017 20:04

solankisamir mentioned this pull request Mar 20, 2017

Add API Version 2016-10-10 and Examples for Deployment Apis #1049

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add version 2016-10-10 for api management #1039

Add version 2016-10-10 for api management #1039

solankisamir commented Mar 16, 2017 •

edited by azuresdkci

Loading

anuchandy left a comment •

edited

Loading

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017 •

edited

Loading

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

solankisamir Mar 18, 2017

anuchandy Mar 17, 2017

anuchandy Mar 17, 2017

anuchandy Mar 17, 2017

anuchandy Mar 18, 2017

anuchandy Mar 18, 2017

solankisamir Mar 18, 2017 •

edited

Loading

anuchandy Mar 18, 2017

solankisamir Mar 18, 2017

anuchandy Mar 18, 2017

solankisamir commented Mar 18, 2017

Add version 2016-10-10 for api management #1039

Add version 2016-10-10 for api management #1039

Conversation

solankisamir commented Mar 16, 2017 • edited by azuresdkci Loading

PR information

Quality of Swagger

anuchandy left a comment • edited Loading

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

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

solankisamir Mar 18, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solankisamir Mar 18, 2017 • edited Loading

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

solankisamir commented Mar 18, 2017

solankisamir commented Mar 16, 2017 •

edited by azuresdkci

Loading

anuchandy left a comment •

edited

Loading

solankisamir Mar 18, 2017 •

edited

Loading

solankisamir Mar 18, 2017 •

edited

Loading