SQL Capabilities Swagger by nathannfan · Pull Request #1015 · Azure/azure-rest-api-specs

nathannfan · 2017-03-09T02:28:33Z

Add swagger for SQL capabilities API and refer to it in capabilities.json.

Validate-spec, validate-example, and linter all passing with no issues.

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

[x ] The title of the PR is clear and informative.
[x ] 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.
[x ] 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.
[x ] 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.

sql.core.json

jaredmoo

Overall looks good, some minor comments

jaredmoo · 2017-03-09T12:42:09Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    "Capabilities"
+                ],
+                "operationId": "Capabilities_Get",
+                "description": "Returns information about the Azure SQL capabilities available for the specified region.",


Remove "information about"

And "Azure SQL"

also please replace 'region' with 'location' throughout this file, e.g.

"Gets the capabilities available for the specified location."

jaredmoo · 2017-03-09T12:42:49Z

arm-sql/2014-04-01/swagger/capabilities.json

+                        "in": "path",
+                        "required": true,
+                        "type": "string",
+                        "description": "The Id of the region for which the Azure SQL capabilities are retrieved."


Remove "Azure SQL"

jaredmoo · 2017-03-09T12:43:43Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    ],
+                    "x-ms-enum": {
+                        "name": "MaxSizeUnits",
+                        "modelAsString": true


Can modelAsString be removed? It's unlikely we will go past petabytes in the foreseeable future

jaredmoo · 2017-03-09T12:44:02Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    ],
+                    "x-ms-enum": {
+                        "name": "CapabilityStatus",
+                        "modelAsString": true


Consider removing?

jaredmoo · 2017-03-09T12:44:43Z

arm-sql/2014-04-01/swagger/capabilities.json

+                        "modelAsString": true
+                    }
+                },
+                "status": {


Can status be extracted as a common definition to reduce duplication?

jaredmoo · 2017-03-09T12:46:44Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    }
+                }
+            },
+            "description": "Represents the maximum size limits for an Azure SQL Database."


"an Azure SQL Database" -> "a database"

jaredmoo · 2017-03-09T12:47:02Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    "items": {
+                        "$ref": "#/definitions/MaxSizeCapability"
+                    },
+                    "description": "The list of supported maximum Azure SQL Database sizes for this Service Objective."


"Azure SQL Database" -> "database"

et cetera for other "Azure SQL" in descriptions other than at top of tile

olydis · 2017-03-09T20:06:12Z

Thanks for providing the example! It seems like one of our validators has uncovered some inconsistencies between the Swagger and the example: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209204805#L975
Please fix the Swagger or the example accordingly.

nathannfan · 2017-03-09T20:19:13Z

@olydis So we have a lot of errors regarding examples having null not matching the expected data types for our resource definitions. Since this is what the API actually returns, we are including the nulls in the examples, and ignoring the example validation errors for now.

I think it's an open discussion here: #798

olydis · 2017-03-09T21:59:06Z

@nathannfan oh, I was not aware of that problem! fair enough, I'm okay with ignoring those null-related messages then, but what about https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209204805#L1650?

olydis · 2017-03-09T22:00:26Z

also, slightly concerned about https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209204805#L2122 (think the Z suffix is missing in order to conform with rfc3339), not sure if our runtime throw up on that!

jaredmoo · 2017-03-09T22:03:30Z

@olydis the system2 error is fixed in #1005

nathannfan · 2017-03-09T22:04:24Z

@olydis For this https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209204805#L2122, we checked, and the client runtimes handle it as UTC time, which is correct in our API's case, so we also ignore these warnings.

jaredmoo · 2017-03-09T23:53:32Z

@olydis well spotted, we'll file this as a bug ('Z' issue)

olydis · 2017-03-10T00:33:02Z

@jaredmoo @nathannfan Excellent, thanks for testing with client runtime!

olydis · 2017-03-10T00:55:47Z

arm-sql/2014-04-01/swagger/capabilities.json

+                        "Default"
+                    ],
+                    "x-ms-enum": {
+                        "name": "CapabilityStatus"


if the set of values has a chance of getting changed in the near future I'd suggest using "modelAsString": true here. (just be aware that otherwise, every change is a breaking change!)

also, please extract the data type you implicitly define here:

type: string enum: ... x-ms-enum: ...

into a top-level definition (with some generic description of what the enum represents) and just $reference it here and in all other places (valuable deduplication, especially in case of a change to the enum!)

None of these are likely to change. I'm happy to put the modelAsString back in if you think we still should.

@jaredmoo are you okay with this?

They should not be modelasstring.

could you please still deduplicate the Swagger? There are 5 identical copies of this enum in here!

olydis · 2017-03-10T00:57:46Z

arm-sql/2014-04-01/swagger/capabilities.json

+            },
+            "description": "Represents the capabilities for a region."
+        },
+        "Resource": {


never referenced - note that you can also cross reference to other swagger files, so even if you plan to use a Resource in here, you can use the one defined in sql.core.json! Deduplication makes both reviewers and maintainers happy ;-)

How do I do this? When I did the initial checkin in October, I asked about this and was told it wasn't possible. I'd be happy to go through and remove all duplicates in another review.

oh things have changed (to the better) since then :-) as per the Swagger standard, you can now give references such as ./sql.core.json#/definitions/Resource

Good to know! I'd like to keep this separate, since capabilities doesn't use Resource at all, but I will fix it in a future PR

@olydis hey Johannes, I tried it out, and the linter is failing.

I moved SqlSubResource and Resource definitions to a Resource.json file, and change the reference to it like this:
"$ref": "./resource.json#/definitions/SqlSubResource"

Validate-spec continues to pass, but I get a linter error back: file: 'file:///e%3A/Azure/azure-rest-api-specs/arm-sql/2014-04-01/swagger/firewallRules.json'
severity: 'Error'
message: 'Error generating client model: Could not find file 'C'
at: '1,1'
source: 'lint'

I'm using linter 0.6.0. Am I doing the reference correctly?

what is "linter 0.6.0."? don't know that tool and sounds like it cannot handle external references

https://github.com/Azure/openapi-lint-extension

olydis · 2017-03-10T00:58:00Z

arm-sql/2014-04-01/swagger/capabilities.json

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


also not used

olydis · 2017-03-10T01:11:50Z

arm-sql/2014-04-01/swagger/capabilities.json

+                "name": {
+                    "readOnly": true,
+                    "type": "string",
+                    "description": "The Service Objective name."


capitalization inconsistent, below you are talking about "server version" and "server edition" which reads well :-)

olydis · 2017-03-10T01:15:38Z

arm-sql/2014-04-01/swagger/capabilities.json

+                        "DTU"
+                    ],
+                    "x-ms-enum": {
+                        "name": "PerformanceLevelUnit"


this again sounds like a good candidate for modelAsString: true or do you want adding a unit (seems likely from looking at it) to break everyone?

Also don't think this is likely to change either. I'm willing to add it, but Azure Team always wants to know why I'm adding modelAsString, and in this case I can't really say it's likely to change any time soon.

@amarzavery can you take a quick look at this? aren't the ARM guidelines the opposite? Or what do you mean with "Azure Team" @nathannfan?

My experience in the past was that we're reluctant to use model-as-string:true unless it's actually likely to change. In this case, I don't think it is.

this seems good to me. DTU is standard measuring unit and another one is not coming soon.

olydis · 2017-03-10T01:15:48Z

arm-sql/2014-04-01/swagger/capabilities.json

+            "description": "Represents the Service Objectives capabilities."
+        },
+        "PerformanceLevel": {            
+            "description": "Represents a possible performance level of a Service Objective Capability.",


So the properties in here are not readonly? :-o

olydis · 2017-03-10T01:16:47Z

arm-sql/2014-04-01/swagger/capabilities.json

+                        "Petabytes"
+                    ],
+                    "x-ms-enum": {
+                        "name": "MaxSizeUnits"


see other comments about enums

olydis · 2017-03-10T01:20:34Z

Apart from the above comments, this looks good to me, the CI issues have been addressed, I see you're discussing the validation messages regarding sql.core.json (where you've just improved descriptions) in #1005 👍

jaredmoo · 2017-03-09T23:39:54Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    "Capabilities"
+                ],
+                "operationId": "Capabilities_Get",
+                "description": "Returns information about the Azure SQL capabilities available for the specified region.",


also please replace 'region' with 'location' throughout this file, e.g.

"Gets the capabilities available for the specified location."

jaredmoo · 2017-03-09T23:41:00Z

arm-sql/2014-04-01/swagger/capabilities.json

+                "limit": {
+                    "readOnly": true,
+                    "type": "integer",
+                    "format": "int32",


will is always be int32? consider int64 here

jaredmoo · 2017-03-10T01:52:17Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    "Capabilities"
+                ],
+                "operationId": "Capabilities_Get",
+                "description": "Returns the capabilities available for the specified region.",


region -> location

jaredmoo · 2017-03-10T01:53:40Z

arm-sql/2014-04-01/swagger/capabilities.json

+                "tags": [
+                    "Capabilities"
+                ],
+                "operationId": "Capabilities_Get",


Isn't this technically Capabilities_List ? Since it returns a list. Or arguably Capabilities_ListByLocation

although, it does (technically) not just return a list, also name and status - otherwise our validation rules should also have caught it.

nathannfan · 2017-03-11T00:41:50Z

Hey @olydis, I'm not sure why the last two commits started failing validation. The error message doesn't say much, and my changes were mostly description updates.

Validate-spec, validate-example, linter and generate.cmd are all running without error as well. Is there a way to drill down deeper?

jaredmoo · 2017-03-11T22:08:25Z

arm-sql/2014-04-01/swagger/capabilities.json

+                        "$ref": "#/parameters/SubscriptionIdParameter"
+                    },
+                    {
+                        "name": "locationID",


Capitalization: locationId ?

jaredmoo · 2017-03-11T22:09:37Z

arm-sql/2014-04-01/swagger/capabilities.json

+            ],
+            "x-ms-enum": {
+                "name": "CapabilityStatus"
+            }


description needed?

good catch, somehow our CI tools didn't catch that!

jaredmoo · 2017-03-11T22:11:20Z

arm-sql/2014-04-01/swagger/sql.core.json

        "edition": {
          "type": "string",
-          "description": "The edition of the database. The DatabaseEditions enumeration contains all the valid editions. If createMode is NonReadableSecondary or OnlineSecondary, this value is ignored.",
+          "description": "The edition of the database. The DatabaseEditions enumeration contains all the valid editions. If createMode is NonReadableSecondary or OnlineSecondary, this value is ignored. To see possible values, query the capabilities API (/subscriptions/{subscriptionId}/providers/Microsoft.Sql/locations/{locationID}/capabilities) referred to by operationId: \"Capabilities_Get.\"",


This is now Capabilities_List

olydis

LGTM (if @jaredmoo's comments are addressed)

nathannfan · 2017-03-14T06:29:34Z

@olydis Hey Johannes, commits have been consistently failing build validation, and I'm not seeing much in the way of error messages in the build pages. Any tips on how to dig deeper?

Locally, it's still passing all validate-spec, validate-example, and successfully generating a client in SDK, which is odd.

anudeepsharma

LGTM

olydis · 2017-03-14T17:55:16Z

@nathannfan that's an internal issue that's hopefully gonna be resolved today

cormacpayne · 2017-03-15T17:02:30Z

Swagger used to generate Azure/azure-sdk-for-net#2914

olydis · 2017-03-15T21:28:05Z

@nathannfan Tooling back up and running: You changing the description of the Resource model made it deviate from the other definitions of Resource (in other files referenced by compositeSql.json). AutoRest therefore rejects compositeSql.json right now! Please either change all other descriptions accordingly (so all definitions of Resource match up 100%) or just reference the Resource of another file instead of defining your own!

…into master_capabilities

jaredmoo · 2017-03-15T22:35:53Z

FYI I'm refactoring our references to Resource in #1005 . So Nathan will make minimal changes to Resource in this PR.

…05-01

nathannfan · 2017-03-15T22:49:13Z

@olydis I see what happened, the conflicting definition of Resource wasn't checked in when I sent the commit with the Resource description change. Should be good to go now.

jaredmoo · 2017-03-15T23:42:09Z

arm-sql/2014-04-01/swagger/capabilities.json

+                "tags": [
+                    "Capabilities"
+                ],
+                "operationId": "Capabilities_List",


According to https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-good-patterns.md section "Consistency in the naming of list methods", this should be Capabilities_ListByLocation.

jaredmoo · 2017-03-15T23:44:47Z

arm-sql/2014-04-01/swagger/capabilities.json

+                "unit": {
+                    "type": "string",
+                    "readOnly": true,
+                    "description": "Unit type used to measure SLO performance level.",


"SLO" -> "service objective"

jaredmoo · 2017-03-15T23:45:26Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    "Capabilities"
+                ],
+                "operationId": "Capabilities_List",
+                "description": "Returns the capabilities available for the specified location.",


Avoid "Returns". How about "Gets"

jaredmoo · 2017-03-15T23:49:39Z

arm-sql/2014-04-01/swagger/capabilities.json

+                        "in": "path",
+                        "required": true,
+                        "type": "string",
+                        "description": "The Id of the location for which the capabilities are retrieved."


The location id whose capabilities are retrieved

jaredmoo · 2017-03-15T23:49:56Z

arm-sql/2014-04-01/swagger/capabilities.json

+    "definitions": {
+        "CapabilityStatus": {
+            "type": "string",
+            "description": "The availability status of the Capability",


Capability -> capability

jaredmoo · 2017-03-15T23:50:37Z

arm-sql/2014-04-01/swagger/capabilities.json

+                    "$ref": "#/definitions/CapabilityStatus"
+                }
+            },
+            "description": "Represents the maximum size limits for a database."


Remove "represents" here and in other descriptions. It's a useless word

jaredmoo · 2017-03-15T23:50:59Z

arm-sql/2014-04-01/swagger/capabilities.json

+            "description": "Represents the service objectives capabilities."
+        },
+        "PerformanceLevel": {            
+            "description": "Represents a possible performance level of a service objective Capability.",


Capability -> capability

jaredmoo · 2017-03-15T23:52:18Z

arm-sql/2014-04-01/swagger/capabilities.json

+                "status": {
+                    "readOnly": true,
+                    "type": "string",
+                    "description": "The status for the location with respect to Azure SQL.",


Azure SQL Database's status for the location.

nathannfan · 2017-03-17T00:47:44Z

@olydis Thanks, Johannes!

AutorestCI · 2017-03-17T00:48:23Z

No modification for NodeJS

AutorestCI · 2017-03-17T00:48:28Z

Azure/azure-sdk-for-ruby#720

AutorestCI · 2017-03-17T00:48:35Z

Azure/azure-sdk-for-python#1037

nathannfan added 5 commits March 7, 2017 14:53

Initial Microsoft.Sql Capabilities swagger commit

47a8ee6

Minor description changes, add capabilities to compositeSql.json

6e3c3be

Update typo in swagger spec

54cb933

Add reference to capabilities API in

753df8a

sql.core.json

Updated description in capabilities.json

f6654f7

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

azuresdkci assigned olydis Mar 9, 2017

jaredmoo suggested changes Mar 9, 2017

View reviewed changes

nathannfan mentioned this pull request Mar 9, 2017

Initial SQL Capabilities SDK commit, with Capabilities API and comments Azure/azure-sdk-for-net#2914

Closed

8 tasks

Removed 'Azure SQL' where appropriate, removed modelAsString

4d36996

olydis reviewed Mar 10, 2017

View reviewed changes

jaredmoo suggested changes Mar 10, 2017

View reviewed changes

olydis added the Reviewed-FeedbackProvided label Mar 10, 2017

nathannfan added 2 commits March 10, 2017 13:33

Updated documentation, fixed status enum, removed unused definitions

171109e

Consolidate enums, respond to PR comments

cf3693f

jaredmoo approved these changes Mar 11, 2017

View reviewed changes

olydis approved these changes Mar 14, 2017

View reviewed changes

nathannfan added 3 commits March 13, 2017 17:58

Update descriptions

38f50fb

Move description to appropriate spot

a93a751

Updated capabilities example

bebf2b2

anudeepsharma approved these changes Mar 14, 2017

View reviewed changes

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs …

fe325f4

…into master_capabilities

nathannfan added 2 commits March 15, 2017 15:37

Change Resource definition back to match Resource definition in 2015-…

29d4011

…05-01

Change Resource definition back to match Resource definition in 2015-…

2256a5f

…05-01

jaredmoo reviewed Mar 15, 2017

View reviewed changes

Updated descriptions and operation id

b8209d5

olydis merged commit 7d9b6c0 into Azure:master Mar 17, 2017

AutorestCI mentioned this pull request Mar 17, 2017

Automatic PR from RestAPI-PR1015 Azure/azure-sdk-for-ruby#720

Merged

AutorestCI mentioned this pull request Mar 17, 2017

[SQL]SQL Capabilities Swagger Azure/azure-sdk-for-python#1037

Closed

Conversation

nathannfan commented Mar 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!

jaredmoo left a comment • 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!

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!

olydis commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathannfan commented Mar 9, 2017

Uh oh!

olydis commented Mar 9, 2017

Uh oh!

olydis commented Mar 9, 2017

Uh oh!

jaredmoo commented Mar 9, 2017

Uh oh!

nathannfan commented Mar 9, 2017

Uh oh!

jaredmoo commented Mar 9, 2017

Uh oh!

olydis commented Mar 10, 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!

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

nathannfan commented Mar 9, 2017 •

edited by azuresdkci

Loading

jaredmoo left a comment •

edited

Loading

olydis commented Mar 9, 2017 •

edited

Loading

nathannfan commented Mar 11, 2017 •

edited

Loading