Cognitive Services swagger api-version 2017-04-18 initial commit by felixwa · Pull Request #1090 · Azure/azure-rest-api-specs

felixwa · 2017-03-31T09:04:01Z

Support Cognitive Services RP version - 2017-04-18
Add following operations and related parameter definitions:
a. POST updateAccountsCreationSettings
b. GET accountsCreationSettings
c. POST checkSkuAvailability
d. Get operations
Fix all issues reported by AutoRest/Validate-Spec/Validate-Example.
Examples are removed from this swagger file – will add them to
dedicated example folder later.

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.

1. Support Cognitive Service RP version - 2017-04-18 2. Add following operations and related parameter definitions: a. POST updateAccountsCreationSettings b. GET accountsCreationSettings c. POST checkSkuAvailability d. Get operations 3. Fix all issues reported by AutoRest/Validate-Spec/Validate-Example. 4. Examples are removed from this swagger file – will add them to dedicated example folder later.

xiaomin-ms · 2017-03-31T11:39:04Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+            "required": true,
+            "x-ms-client-flatten": true,
+            "schema": {
+              "$ref": "#/definitions/CognitiveServicesResourceKindSettingUpdateParameter"


ResourceKindSetting => AccountsCreationSettings?

xiaomin-ms · 2017-03-31T11:40:21Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+        "allowCreate": {
+          "type": "string",
+          "description": "Whether allow user to create resource.",
+          "enum": [


Does it support Boolean type?

It supports but AutoRest suggests to use Enum instead of Boolean.

the suggestion from AutoRest is to evaluate if the property is really a boolean or not, the intent is to consider if there could be more than 2 values possible for the property in the future or not, if the answer is no, then a boolean is fine, if the answer is yes, there could be other values, then make it an enum with values that represent what those should be. The idea is that it can help avoid breaking changes if you want to add one more possible value, naming the values "true", "false" I believe defeats this purpose.

Changed to Boolean.

I suggest AutoRest can turn off this warning though.

salameer · 2017-04-03T22:50:06Z

Hi @felixwa

@alvadb will be reviewing this PR very shortly, thanks for your patience! :)

veronicagg · 2017-04-04T21:40:47Z

@felixwa thanks for submitting this PR.
If this spec is based on a previous api-version:
"When you are creating the swagger spec for the new api-version, please copy the swagger spec from the previous version in to the new api-versioned folder and commit it. After that overwrite it with the changes for the new api-version. This makes it easy for us to review the changes."
(https://github.com/Azure/azure-rest-api-specs/blob/c12accd4df379643a3aa9c5bf385e6f6ef50bf73/.github/CONTRIBUTING.md#filenames-and-folder-structure)
It's great to see linter validation is clean. Please consider if you'll be adding descriptions for the remaining warnings, as those would help with documentation (https://travis-ci.org/Azure/azure-rest-api-specs/jobs/217096052#L1022).
For a new api-version, we're requiring x-ms-examples, to make sure validation is clean against those.

veronicagg

Comments inline.

veronicagg · 2017-04-04T21:09:17Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+            "pattern": "^[a-zA-Z0-9][a-zA-Z0-9_.-]*$"
+          },
+          {
+            "name": "body",


should this be named "parameters"?

Changed all places to "parameters".

veronicagg · 2017-04-04T21:11:35Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+      "post": {
+        "tags": [ "CognitiveServicesAccounts" ],
+        "operationId": "CheckSkuAvailability_List",
+        "description": "Check availalable SKUs.",


typo in "available"

veronicagg · 2017-04-04T21:12:19Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+          "200": {
+            "description": "OK.",
+            "schema": {
+              "$ref": "#/definitions/CheckSkuAvailabilityResultList"


is this a pageable operation?

veronicagg · 2017-04-04T21:13:58Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+    "/subscriptions/{subscriptionId}/providers/Microsoft.CognitiveServices/locations/{location}/updateAccountsCreationSettings": {
+      "post": {
+        "tags": [ "CognitiveServicesAccounts" ],
+        "operationId": "CognitiveServices_UpdateResourceKindSettings",


is this operation id accurate for the operation path?

veronicagg · 2017-04-04T21:14:15Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+            "$ref": "#/parameters/locationParameter"
+          },
+          {
+            "name": "body",


should this be named "parameters"?

Changed all places to "parameters".

veronicagg · 2017-04-04T21:16:23Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+            "required": true,
+            "type": "string",
+            "enum": [ "current" ],
+            "description": "Accounts creation setting name, now only support \"current\".",


are you planning on adding other values? If there's only one value in the enum on a required parameter, then it's value will be taken as a constant (https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#single-value-enum-as-a-constant)
You could just write that value in the path as a constant, if you're not planning on adding more values.

This operation has been changed to POST after more discussions with ARM team. This "current" has gone.

veronicagg · 2017-04-04T21:18:49Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+          "200": {
+            "description": "OK",
+            "schema": {
+              "$ref": "#/definitions/CognitiveServicesResourceKindSettingResult"


is this a pageable operation?

veronicagg · 2017-04-04T21:27:40Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+        "skuAvailable": {
+          "type": "string",
+          "description": "Indicates the given SKU is available or not.",
+          "enum": [


should this be a boolean? are there other values this property may accept?

Changed to Boolean.

veronicagg · 2017-04-04T21:29:51Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+        "allowCreate": {
+          "type": "string",
+          "description": "Whether allow user to create resource.",
+          "enum": [


should this be a boolean? could this property accept other values?

veronicagg · 2017-04-04T21:31:57Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+        "allowCreate": {
+          "type": "string",
+          "description": "Whether allow user to create resource.",
+          "enum": [


the suggestion from AutoRest is to evaluate if the property is really a boolean or not, the intent is to consider if there could be more than 2 values possible for the property in the future or not, if the answer is no, then a boolean is fine, if the answer is yes, there could be other values, then make it an enum with values that represent what those should be. The idea is that it can help avoid breaking changes if you want to add one more possible value, naming the values "true", "false" I believe defeats this purpose.

Commit original 2016-02-01-preview version for comparison purpose

Fix review comments from veronicagg

Fix a typo.

felixwa · 2017-04-06T09:08:53Z

@veronicagg , I have commit 2016-02-01-preview version then made another commit to overwrite it with 2017-04-18 version. Let me know if this works for you.
Also I have added those missing "description" fields - it is strange that AutoRest I use locally did not report them, I'm sure I am using the latest. Does travis-ci use a different version?
As for ms-examples, I know now they should be added to a separated folder. Please help review existing changes first and if they looking good for you I can add them in this PR or a new PR (I prefer to add them in a new PR).

veronicagg · 2017-04-06T18:30:32Z

@felixwa thanks for committing the previous version! Regarding the latest version of autorest, here are the update instructions for autorest: https://github.com/Azure/autorest#updating-autorest (just to make sure you're running the latest - Travis should be using the latest).
The examples (x-ms-examples) are in a separate folder, but need a reference from the swagger spec in each operation. We should do it in this PR, because it helps catch validate the swagger spec, so if there are issues, we want to fix them from the start instead of bug fixes (as this is a new api-version). Thanks!

veronicagg · 2017-04-06T18:43:26Z

@felixwa should you be adding "securityDefinitions" section, similar to https://github.com/Azure/azure-rest-api-specs/blob/master/arm-analysisservices/2016-05-16/swagger/analysisservices.json#L26 ?

veronicagg · 2017-04-06T18:54:55Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+            "in": "body",
+            "required": true,
+            "schema": {
+              "$ref": "#/definitions/CognitiveServicesAccountCreateParameters"


CognitiveServicesAccountCreateParameters [](start = 37, length = 40)

Is there a reason for the body parameter and the response not being the same model? Please consider this rule" "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)

@veronicagg , we followed RPC v2. In PUT body, there is no need to put id/name/type etc. there because they are already there in URL.

@felixwa would you consider changing this in the future? Users of the api won't be able to reuse the same identity between GET and PUT ("This ensures that the same entity will be reusable between GET and PUT." ). There will be an error shown by our validation tools on this soon.

Ok we could consider that if the tool will report this anyway. Will that be a breaking change for RP and/or this swagger file?

It would be a breaking change for the SDK, so it'd need to be done consciously.

veronicagg · 2017-04-06T18:59:29Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+      "properties": {
+        "sku": {
+          "$ref": "#/definitions/Sku",
+          "description": "Required. Gets or sets the SKU of the resource."


Please take a look at: "M2016: For PATCH operation, every property in the model for the request body MUST be optional and the property MUST NOT provide any default value. The absence of default values applies recursively to complex properties in the model definition." (from https://github.com/Azure/azure-rest-api-specs/blob/62c96876795e600de8369efe42edd3a70f494017/documentation/swagger-checklist.md)
The description says this is required, I don't this marked as required, but the Sku model has "name" as required.

Not required. I have revised the "description".

veronicagg · 2017-04-06T19:01:29Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+      ],
+      "description": "The SKU of the cognitive services account."
+    },
+    "CognitiveServicesAccountPropertiesCreateParameters": {


CognitiveServicesAccountPropertiesCreateParameters [](start = 5, length = 50)

is there no type for this model? object?

This is just a placeholder actually. The RP SDK requires this field but we don't have any properties in it.

I'm not sure I understand this, could you clarify? "properties" is also a required property, what would the user input to this property? what would you include in your example for this case?

Like below:

{
"sku":{
"name": "S0"
},
"location": "westeurope",
"kind": "Face",
"properties": {}
}

User would input nothing in the "properties" field. For our RP it is not required, it is just RP SDK requires that field present there.

veronicagg · 2017-04-06T19:02:12Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

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


CognitiveServicesAccountPropertiesCreateParameters [](start = 33, length = 50)

what is this flattening? I don't see any properties in the referenced model.

Removed (but AutoRest reports a new warning).

veronicagg

Please see some more questions/comments inline.

1. Change account name max/min length to be consistent with UI. 2. Resolved some review comments. 3. Fix some issues with CheckSkuAvailability operation.

felixwa · 2017-04-07T09:19:42Z

@veronicagg , I have added securityDefinitions and resolved some review comments.
I can add examples in this PR after we agree on left open ones.

veronicagg

Please see reply to a couple of your comments.

Adding examples for every operation. Fix some issues found when adding examples.

felixwa · 2017-04-10T09:58:02Z

@veronicagg , I have added examples for every operation.

felixwa · 2017-04-10T10:02:28Z

arm-cognitiveservices/2017-04-18/swagger/cognitiveservices.json

+        "message": {
+          "type": "string",
+          "description": "Additional error message."
+        }


@veronicagg , I noticed that validate-example reports warnings for these 2 fields because the in example they are null. I tried to make the type ["string", "null"] but then AutoRest reports errors then.
Please advice how to make tools happy in this case.

@felixwa you may be hitting #798, if so, please ignore the reported error for now.

veronicagg · 2017-04-10T20:36:52Z

Regarding the "properties" that I asked before, is there a reason why there is a $ref in that case and not something like?
"properties": {
"type": "object",
"additionalProperties": {
"type": "string"
},
"description": "Must exist in the request. Must not be null."
}

or even without "additionalProperties"?
Also, you may want to indicate in the description that an empty object must be sent in this case.

Remove CognitiveServicesAccountPropertiesCreateParameters, make it "inline" and refine description. We still need it because currently the underlying SDK requires it.

felixwa · 2017-04-11T07:47:33Z

@veronicagg , I have removed that CognitiveServicesAccountPropertiesCreateParameters and make it "inline" as you suggested.

AutorestCI · 2017-04-11T18:35:22Z

No modification for Ruby

AutorestCI · 2017-04-11T18:35:28Z

No modification for NodeJS

AutorestCI · 2017-04-11T18:36:50Z

No modification for Python

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

azuresdkci assigned alvadb Mar 31, 2017

xiaomin-ms reviewed Mar 31, 2017

View reviewed changes

salameer added the Reassign label Apr 4, 2017

salameer unassigned alvadb Apr 4, 2017

azuresdkci removed the Reassign label Apr 4, 2017

azuresdkci assigned veronicagg Apr 4, 2017

veronicagg suggested changes Apr 4, 2017

View reviewed changes

felixwa added 3 commits April 6, 2017 16:42

Commit 2016-02-01-preview version for comparison

4361526

Commit original 2016-02-01-preview version for comparison purpose

Fix review comments

71696f2

Fix review comments from veronicagg

Fix a typo

29d8b75

Fix a typo.

veronicagg reviewed Apr 6, 2017

View reviewed changes

veronicagg suggested changes Apr 6, 2017

View reviewed changes

Fix some issues and review comments

30b02ba

1. Change account name max/min length to be consistent with UI. 2. Resolved some review comments. 3. Fix some issues with CheckSkuAvailability operation.

veronicagg suggested changes Apr 7, 2017

View reviewed changes

Adding examples

3679917

Adding examples for every operation. Fix some issues found when adding examples.

felixwa commented Apr 10, 2017

View reviewed changes

lmazuel mentioned this pull request Apr 10, 2017

[Cognitive Service] properties in azure.mgmt.cognitiveservices.models.CognitiveServicesAccountCreateParameters Azure/azure-sdk-for-python#1060

Closed

Remove CognitiveServicesAccountPropertiesCreateParameters

1e96403

Remove CognitiveServicesAccountPropertiesCreateParameters, make it "inline" and refine description. We still need it because currently the underlying SDK requires it.

veronicagg approved these changes Apr 11, 2017

View reviewed changes

veronicagg merged commit f1786a1 into Azure:master Apr 11, 2017

This was referenced May 15, 2017

Fixes issues with CheckSkuAvailability operation #1224

Merged

Cognitive Services version 2017-04-18 Azure/azure-sdk-for-net#3221

Merged

Conversation

felixwa commented Mar 31, 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!

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!

salameer commented Apr 3, 2017

Uh oh!

veronicagg commented Apr 4, 2017

Uh oh!

veronicagg 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!

felixwa commented Apr 6, 2017

Uh oh!

veronicagg commented Apr 6, 2017

Uh oh!

veronicagg commented Apr 6, 2017

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!

veronicagg Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

felixwa commented Mar 31, 2017 •

edited by azuresdkci

Loading

veronicagg Apr 6, 2017 •

edited

Loading

veronicagg Apr 6, 2017 •

edited

Loading

{
"sku":{
"name": "S0"
},
"location": "westeurope",
"kind": "Face",
"properties": {}
}