Removed no longer supported SQL resource types and renamed several operations by jaredmoo · Pull Request #1005 · Azure/azure-rest-api-specs

jaredmoo · 2017-03-07T03:46:20Z

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.

These APIs were used to upgrade servers from v2 to v12. This is no longer applicable because all servers are v12.

Same for ElasticPoolEditions. Enum names should be singular.

msftclas · 2017-03-07T03:46:24Z

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

jaredmoo · 2017-03-07T20:11:54Z

Clarification: I actually removed inaccessible or no longer supported types, not APIs. The enum rename is included to get breaking changes done all together.

veronicagg

Thanks @jaredmoo Your changes look good to me, I've labeled this PR as "potential-sdk-breaking-changes".
There are other errors listed by our validation tool, not related to the changes in this PR, would you like to take a look at those as part of this PR?

jaredmoo · 2017-03-07T20:54:29Z

Thanks @veronicagg , I assume you're talking about the following checks that have PR_ONLY = TRUE?
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208461582
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208461584

jaredmoo · 2017-03-07T20:58:13Z

I believe we are waiting on you guys to make changes to the examples validation, so I will just focus on the linter errors ( https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208461582 )

jaredmoo · 2017-03-07T21:14:25Z

Comments on errors:

ListByOperationsValidation: I believe the existing names are correct and we should silence these warnings somehow.
ProvidersPathValidation: in these case current is a singleton, I'm interested in your guidance on this. Should we make it a {param} with only one allowed value (current?)
BodyTopLevelProperties: isn't kind a standard property? Either way this property is what the API has which can't be changed
ArmResourcePropertiesBag: I don't understand what this is saying. It sounds like it's complaining that we are replacing the base properties with our own properties. Can you clarify how to fix this?
BooleanPropertyNotRecommended, GuidValidation, DefinitionsPropertiesNamesCamelCase: This is what the API does, I can't change that. We should silence these warnings.
TrackedResourceValidation: Let me check if this is actually a tracked resource.
TrackedResourcePatchOperationValidation: Understood, I believe patch is already supported. Will investigate.

This is the output of latest version of AutoRest that I'm looking at, right?

veronicagg · 2017-03-08T18:47:53Z

@jaredmoo Regarding linter errors:

ListByOperationsValidation - I agree.
ProvidersPathValidation - Yes, so the recommendation would be to add a parameter, and make it an enum with 1 value (https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-extensions.md#single-value-enum-as-a-constant)
BodyTopLevelProperties - Ok, @ravbhatnagar is kind a standard property?
ArmResourcePropertiesBag - ignore this one for now, we've temporarily removed this rule, as it needs modifications
BooleanPropertyNotRecommended, GuidValidation, DefinitionsPropertiesNamesCamelCase - Ok, let's keep it as is for now.
TrackedResouceValidation - sounds good
TrackedResourcePatchOperationValidation - sounds good

Regarding model validation errors:

We may have missed this error before: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208461584#L1640, there looks to be an extra "2" in https://github.com/Azure/azure-rest-api-specs/blob/master/arm-sql/2014-04-01/examples/DatabaseList.json#L22
For the rest, yes, we agreed on suppressing those.

Thanks!

veronicagg

Please take a look at my previous comment and let me know if you have additional questions. Thanks!

I had removed this in 80ebb4c

jaredmoo · 2017-03-08T19:26:28Z

Done. System2 is actually correct, I had removed this enum element previous because I thought it was wrong.

jaredmoo · 2017-03-08T19:27:35Z

Will there be a way of suppressing these errors that are acceptable? That way in the future it will be easier to catch regressions.

anudeepsharma

LGTM

There is no other way to get a server, so the simplest name makes sense.

veronicagg

Left one more comment inline, that's giving an error during semantic validation.
Regarding the "exceptions", yes, we will have a way to suppress them in the future, we're working on improving the tools to support that.
Regarding the remaining errors:

What did you find out on the TrackedResource checks?
BodyTopLevelProperties - kind is a standard property and we'll be correcting the rule

Since the latest changes introduced "importExport.json" to the PR, you may want to take a look at validation failures:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209193852#L964

veronicagg · 2017-03-09T18:42:02Z

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

            "type": "string",
            "description": "The name of the database for which setting the transparent data encryption applies."
          },
+          {


A parameter reference like this one is missing for the "get" operation below at https://github.com/Azure/azure-rest-api-specs/pull/1005/files#diff-79c4f982670d02607125a6b94696be04R1379
See https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209193853#L973

Fixed in 7cf6aee

jaredmoo · 2017-03-09T21:10:52Z

TrackedResource checks: yes these resources (server, db, pool) support PATCH. We will do this in a later PR. Opened issue [SQL] Add documentation of PATCH for tracked entities #1018
ImportExport errors: fixing those now.

jaredmoo · 2017-03-09T21:12:59Z

What do I do about this one? https://travis-ci.org/Azure/azure-rest-api-specs/jobs/209193852#L980

We already have /providers/Microsoft.Sql/operations defined in sql.core.json.

jaredmoo · 2017-03-15T19:45:39Z

We have some proxy resources that don't have a type property. IMO this is an API bug, but for Swagger purposes right now should I have them include ProxyResource, or make my own model type e.g. SubResource which has just id and name?

veronicagg

Regarding your question on "ProxyResource" versus "SubResource", since you're referencing your own definitions right now, you can create your own.
Regarding the enum mismatch from a previous comment, I'd recommend updating the spec to make it a "string" and include in the description what the possible values are and how to construct the string, not sure if they are comma separated or semicolon separated, and also, is there a semicolon at the end? When the API bug gets fixed, then this could be turned into an array type.

veronicagg · 2017-03-15T22:22:29Z

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

@@ -0,0 +1,271 @@
+{  
+  "definitions":{  
+    "Resource":{  


missing "x-ms-azure-resource": true

veronicagg · 2017-03-15T22:42:03Z

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

+        }
+      ]
+    },
+    "ResourceModelWithAllowedPropertySet":{  


missing "x-ms-azure-resource": true?

Removed this model (unused)

veronicagg · 2017-03-15T22:54:36Z

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

+      ]
+    }
+  },
+  "parameters":{  


are you referring to these parameters anywhere?

No, removed

I was just copying resourceDefinitions.json from Azure/azure-rest-api-specs-pr#40 since I thought that's what you were asking for.

veronicagg · 2017-03-15T22:56:33Z

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

@@ -0,0 +1,271 @@
+{  


you could include these definitions in one of your existing files and have others refer to them, having a separate file will, at this time, produce "many" violations, since this swagger file is not following an RP structure. At some point, our tools would account for this separate files.

Ok. I misunderstood what you said earlier. Done.

Resource is back to being defined in each spec file. Also improved some operation descriptions.

jaredmoo · 2017-03-16T00:10:50Z

Changed that enum to a regular string. It's semicolon separated, description was wrong.

veronicagg · 2017-03-17T00:59:20Z

Thanks @jaredmoo .
Regarding the resource definitions, if they are shared among your own files, you could have them just in one of those, and refer to the ones in that one from the others. It's also fine if you want to keep it this way.
From the latest validations, have you looked into the following?

jaredmoo · 2017-03-17T01:27:44Z

https://travis-ci.org/Azure/azure-rest-api-specs/jobs/211678413#L935 - false positive. DatabaseSecurityAlertPolicy has location, but it's not required, so it's not a tracked resource.

https://travis-ci.org/Azure/azure-rest-api-specs/jobs/211678413#L1060 - unfortunately the API already uses guid in production, I can't change it

https://travis-ci.org/Azure/azure-rest-api-specs/jobs/211678413#L1239 - issue #1018

Resolving merge conflicts and removing resource definitions from all files except sql.core
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/211678413#L1250 - issue #1018

https://travis-ci.org/Azure/azure-rest-api-specs/jobs/211678413#L1272 - unfortunately the API already uses guid in production, I can't change it

Refer to sql.core.json definitions from other files.

veronicagg

Changes look good, thanks!

AutorestCI · 2017-03-17T17:08:21Z

No modification for NodeJS

AutorestCI · 2017-03-17T17:08:37Z

Azure/azure-sdk-for-ruby#721

AutorestCI · 2017-03-17T17:08:48Z

Azure/azure-sdk-for-python#1039

jaredmoo added 3 commits March 6, 2017 17:36

Removed upgrade APIs which are no longer useful.

192723c

These APIs were used to upgrade servers from v2 to v12. This is no longer applicable because all servers are v12.

Removed unsupported schemas APIs

e1d3c29

Ranemd DatabaseEditions enum to DatabaseEdition.

e609ab3

Same for ElasticPoolEditions. Enum names should be singular.

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

azuresdkci assigned veronicagg Mar 7, 2017

jaredmoo changed the title ~~Removed no longer supported SQL APIs~~ Removed no longer supported SQL resource types Mar 7, 2017

veronicagg added the potential-sdk-breaking-change label Mar 7, 2017

veronicagg reviewed Mar 7, 2017

View reviewed changes

veronicagg suggested changes Mar 8, 2017

View reviewed changes

jaredmoo added 3 commits March 8, 2017 11:07

Make TDE '/current' an enum param

4c35ae9

TDE config is method param

f10bbd3

Reinstated System2 SLO.

4c42a2e

I had removed this in 80ebb4c

anudeepsharma reviewed Mar 8, 2017

View reviewed changes

nathannfan approved these changes Mar 8, 2017

View reviewed changes

jaredmoo added 3 commits March 8, 2017 17:32

Renamed Servers_GetByResourceGroup to Servers_Get.

9bc4a75

There is no other way to get a server, so the simplest name makes sense.

Add $expand to list databases operation

4289a16

Make DB activity operation id format as uuid

e25129c

veronicagg suggested changes Mar 9, 2017

View reviewed changes

jaredmoo added 2 commits March 9, 2017 12:28

Added missing TDE config name parameter

7cf6aee

Merge remote-tracking branch 'azure/master' into breakingchanges

cac3600

jaredmoo added 2 commits March 15, 2017 12:23

Use common resource definitions

22b1691

Added missing location props

c34b1f2

Add another missing location

02a3170

jaredmoo force-pushed the breakingchanges branch from 73505a0 to 02a3170 Compare March 15, 2017 20:12

veronicagg suggested changes Mar 15, 2017

View reviewed changes

jaredmoo added 3 commits March 15, 2017 17:01

Remove common Resource definition files.

008e920

Resource is back to being defined in each spec file. Also improved some operation descriptions.

Change disabledAlerts back to string, not enum.

5726dc6

Fixed body parameters name in examples

549bc4e

jaredmoo added 2 commits March 15, 2017 17:55

storageAccountAccessKey is write-only

8235db4

Fixed more incorrectly named enums

ac81309

jaredmoo added 2 commits March 16, 2017 18:20

Merge remote-tracking branch 'azure/master' into breakingchanges

5938b3a

Capabilities_List -> ListByLocation in description

c778824

jaredmoo added 3 commits March 16, 2017 18:32

Remove repeated Resource definitions.

a8af2c3

Refer to sql.core.json definitions from other files.

Reverted resource property descriptions changes.

2edaf28

Removed 'DataWarehouse' from Pause/Resume op ids.

c60590e

veronicagg approved these changes Mar 17, 2017

View reviewed changes

veronicagg merged commit ea48f56 into Azure:master Mar 17, 2017

AutorestCI mentioned this pull request Mar 17, 2017

Automatic PR from RestAPI-PR1005 Azure/azure-sdk-for-ruby#721

Closed

AutorestCI mentioned this pull request Mar 17, 2017

[SQL] Removed no longer supported SQL resource types and renamed several operations Azure/azure-sdk-for-python#1039

Closed

jaredmoo mentioned this pull request Mar 29, 2017

Add Azure SQL Database Failover Group REST API specs #1079

Merged

10 tasks

jaredmoo mentioned this pull request Apr 5, 2017

Adding swagger changes for Virtual Network Rules #1095

Merged

10 tasks

jaredmoo deleted the breakingchanges branch July 8, 2017 17:21

Conversation

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

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented Mar 7, 2017

Uh oh!

jaredmoo commented Mar 7, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

jaredmoo commented Mar 7, 2017

Uh oh!

jaredmoo commented Mar 7, 2017

Uh oh!

jaredmoo commented Mar 7, 2017

Uh oh!

veronicagg commented Mar 8, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

jaredmoo commented Mar 8, 2017

Uh oh!

jaredmoo commented Mar 8, 2017

Uh oh!

anudeepsharma left a comment

Choose a reason for hiding this comment

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!

jaredmoo commented Mar 9, 2017

Uh oh!

jaredmoo commented Mar 9, 2017

Uh oh!

jaredmoo commented Mar 15, 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!

jaredmoo commented Mar 16, 2017

Uh oh!

veronicagg commented Mar 17, 2017

Uh oh!

jaredmoo commented Mar 17, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

AutorestCI commented Mar 17, 2017

Uh oh!

AutorestCI commented Mar 17, 2017

Uh oh!

AutorestCI commented Mar 17, 2017

Uh oh!

jaredmoo commented Mar 7, 2017 •

edited

Loading