[KeyVault] Updating resource-manager swagger to include missing defintions by schaabs · Pull Request #2210 · Azure/azure-rest-api-specs

schaabs · 2018-01-03T17:24: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.

jianghaolu · 2018-01-03T22:59:09Z

+        }
+    },
+    "responses":{
+        "200":{


Vaults_CreateOrUpdate can return 201 too, please provide example response.

jianghaolu · 2018-01-03T23:00:09Z

+        }
+    },
+    "responses":{
+        "200":{


Vaults_Update can return 201 too, please provide example response.

jianghaolu · 2018-01-03T23:18:46Z

+       "subscriptionId":"00000000-0000-0000-0000-000000000000",
+       "api-version":"2016-10-01",
+       "parameters":{
+            "location": "westus", 


Location isn't defined in VaultPatchParameters.

jianghaolu · 2018-01-03T23:20:40Z

+    "responses":{
+        "200":{
+            "headers":{},
+            "body":{


In Vaults_delete 200 doesn't return a body.

jianghaolu · 2018-01-03T23:21:07Z

+        }
+     },
+    "responses": {
+        "200": {


Please add 201 response example too.

jianghaolu · 2018-01-03T23:45:05Z

+        }
+    },
+    "responses": {
+        "200": {


Please add 201 response example.

Sorry I missed here - also add 201 here. Signed off otherwise.

jianghaolu · 2018-01-03T23:45:22Z

+                        "enabled": true,
+                        "created": 1514940684,
+                        "updated": 1514940698
+                    },


Missing required property "value".

value is not actually required in this model. I've update the model to reflect that.

jianghaolu · 2018-01-03T23:45:55Z

+                        "enabled": true,
+                        "created": 1514940950,
+                        "updated": 1514940950
+                    },


Missing required property "value".

Same for listSecrets.json

value is not actually required in this model. I've update the model to reflect that.

jianghaolu · 2018-01-03T23:47:47Z

+          "description": "Display name of log specification."
+        },
+        "blobDuration": {
+          "type": "string",


Is this the standard TimeSpan? If so, please add "format": "duration".

jianghaolu · 2018-01-03T23:48:53Z

+        "message": {
+          "readOnly": true,
+          "type": "string",
+          "description": "Gets an error message explaining the Reason value in more detail."


Remove all the "Gets", "Sets", or "Gets or sets" in property descriptions.

jianghaolu · 2018-01-03T23:54:32Z

Found some issues in the examples. The spec is mostly good.

…eration

schaabs · 2018-01-04T02:36:19Z

@jianghaolu All of your feedback should have been addressed here. Please let me know if you have any other concerns.

jianghaolu · 2018-01-04T03:46:18Z

@schaabs Thanks! This looks great other than one more comment here: https://github.com/Azure/azure-rest-api-specs/pull/2210/files/462e482ff6ad09a1826f2c0e04c5b7b0f027cba7#diff-713ba0219ac4cf6d8bcb48ffeb9c30cc

ravbhatnagar · 2018-01-04T06:36:30Z

+          "type": {
+          "readOnly": true,
+          "type": "string",
+          "description": "The resource name of access policy."


typo in description

ravbhatnagar · 2018-01-04T06:36:54Z

+          "type": "string",
+          "description": "The resource name of access policy."
+        },
+          "location": {


Why is location readOnly? Is this a tracked resource or proxy only?

This is a proxy to access policies which are defined on a vault which is a tracked resource. The value for location is derived from the vault and not directly settable.

lets remove the location property then.

azuresdkciprbot · 2018-01-05T00:26:38Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/keyvault/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 12
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2018-01-05T00:52:31Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/keyvault/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 12
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

ravbhatnagar · 2018-01-05T01:21:51Z

There were a few issues in these APIs that were discussed over email. We got an ACK from the Keyvault team on the issues and they are aware and fine with merging as is. Hence signing off.

salameer · 2018-01-05T01:49:55Z

Merging this Gaurav and Jianghao are good here

AutorestCI · 2018-01-05T01:54:30Z

Azure/azure-sdk-for-ruby#1218

AutorestCI · 2018-01-05T02:25:19Z

Azure/azure-sdk-for-python#1735

Scott Schaab added 2 commits January 3, 2018 09:15

updating keyvault RM swagger to include missing definitions and examples

0bcffea

fixing updated paths in readme.md

bece2a3

schaabs requested review from RandalliLama, dragav and seanbamsft and removed request for dragav January 3, 2018 17:25

azuresdkci assigned jianghaolu Jan 3, 2018

Scott Schaab added 2 commits January 3, 2018 09:42

moving attributes definition into secrets.json

56bc236

referencing Resource definition from keyvault.json in secrets.json

462e482

schaabs force-pushed the akv-mgmtfull branch from 2c5c42a to 462e482 Compare January 3, 2018 20:58

jianghaolu suggested changes Jan 3, 2018

View reviewed changes

Scott Schaab added 5 commits January 3, 2018 17:30

updates addressing PR feedback

e5f5687

fixing linting issues introduced with updates

03e395d

moving listOperations method to providers.json as it's not a vault op…

b2ff292

…eration

adding providers.json to readme.md

c799ef9

fixing syntax error in keyvault.json

22a028c

ravbhatnagar reviewed Jan 4, 2018

View reviewed changes

Scott Schaab added 3 commits January 4, 2018 14:52

fixing typos in property descriptions

be28391

fixing parameter name in example checkVaultNameAvailability.json

3c6661a

updating descriptions and examples based on PR feedback

7e2d938

jianghaolu approved these changes Jan 5, 2018

View reviewed changes

adding 201 response to updateSecret.json example

152da1e

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Jan 5, 2018

salameer merged commit ded42c8 into Azure:master Jan 5, 2018

AutorestCI mentioned this pull request Jan 5, 2018

Automatic PR from RestAPI-PR2210 Azure/azure-sdk-for-ruby#1218

Closed

AutorestCI mentioned this pull request Jan 5, 2018

[KeyVault] Updating resource-manager swagger to include missing defintion Azure/azure-sdk-for-python#1735

Closed

schaabs mentioned this pull request Feb 9, 2018

[Key Vault] reverting changes which introduce breaking changes to generated SDKs #2462

Merged

10 tasks

Conversation

schaabs commented Jan 3, 2018 • 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!

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!

jianghaolu commented Jan 3, 2018

Uh oh!

schaabs commented Jan 4, 2018

Uh oh!

jianghaolu commented Jan 4, 2018

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!

azuresdkciprbot commented Jan 5, 2018

Uh oh!

azuresdkciprbot commented Jan 5, 2018

Uh oh!

ravbhatnagar commented Jan 5, 2018

Uh oh!

salameer commented Jan 5, 2018

Uh oh!

AutorestCI commented Jan 5, 2018

Uh oh!

AutorestCI commented Jan 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

schaabs commented Jan 3, 2018 •

edited by azuresdkci

Loading