SQL database, elastic pool, and capabilities 2017-10-01-preview #2734

jaredmoo · 2018-03-22T04:05:44Z

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.

Rename 2017-03 ArmSku->Sku, Status->DatabaseStatus and added OnlineSecondary CreateMode. These changes should make 2017-03 db client compatible with 2017-10 db.

Introduced both recommendedElasticPools.json and recommendedElasticPoolsDecoupled.json. These files are identical except that recommendedElasticPools.json references 2014 Database resource, recommendedElasticPoolsDecoupled.json references TrackedResource so that it can be versioned independently of database api.

AutorestCI · 2018-03-22T04:08:00Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2285

Differences against package-composite-v2: - 2014-04-01/recommendedElasticPoolsDecoupled.json used instead of 2014-04-01/recommendedElasticPools.json so that recommendedElasticPools is separated from databases. This allows us to update database and recommendedElasticPools apis independently. - databases.json, elasticPools.json, and capabilities.json updated from 2014-04-01 to 2017-10-01-preview versions. - 2017-03-01-preview/renameDatabase.json removed since the rename api is there in 2017-10-01-preview/databases.json

jaredmoo · 2018-03-22T04:09:17Z

The service is not yet fully deployed, but it is ok to merge because package-composite-v3 is not yet default tag. When the service is available in production, @payiAzure will issue follow-up PR where she updates the default tag to package-composite-v3.

AutorestCI · 2018-03-22T04:10:32Z

Automation for azure-libraries-for-java

The initial PR has been merged into your service PR:
AutorestCI/azure-libraries-for-java#9

AutorestCI · 2018-03-22T04:11:32Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#1449

AutorestCI · 2018-03-22T04:13:17Z

Automation for azure-sdk-for-node

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-node#2489

jaredmoo · 2018-03-22T05:17:28Z

There are some model validation errors in RecommendedElasticPools_ListByServer that I am not planning to fix. The model change is deliberate in order to decouple Database API from RecommendedElasticPools.

mcardosos · 2018-03-23T22:18:53Z

specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/capabilities.json

+          "description": "The name of the SKU, typically, a letter + Number code, e.g. P3.",
+          "type": "string"
+        },
+        "tier": {


tier [](start = 9, length = 4)

Could this be an enum?

Will do in another PR later.

mcardosos · 2018-03-23T22:23:24Z

specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/capabilities.json

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


parameters [](start = 3, length = 10)

There are many parameters in this list that are not used in this swagger in particular. I they are not used in other swaggers by reference, the best would be to remove them, to keep swaggers cleaner

Understood but these swaggers are generated, it is easier to keep the extra parameters.

mcardosos · 2018-03-23T22:31:08Z

specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/databases.json

+          "description": "The name of the SKU, typically, a letter + Number code, e.g. P3.",
+          "type": "string"
+        },
+        "tier": {


tier [](start = 9, length = 4)

Same here, I believe this could be an enum

Will do in future PR.

mcardosos · 2018-03-23T22:33:14Z

specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/databases.json

+        }
+      }
+    },
+    "Sku": {


Sku [](start = 5, length = 3)

Looks like this definition is also present in the capabilities folder. Is it a good idea to define it just one and reference it form other files? WDYT?

Will do in future PR.

mcardosos · 2018-03-23T22:39:30Z

specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/databases.json

+          "format": "uuid",
+          "description": "The ID of the database.",
+          "type": "string",
+


currentSku [](start = 9, length = 10)

This question is out of curiosity. Is there really a sku property under properties and another under database?

mcardosos · 2018-03-23T22:43:20Z

specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/databases.json

+          "format": "uuid",
+          "description": "The ID of the database.",
+          "type": "string",
+


parameters [](start = 3, length = 10)

Same here. There are many parameters not used here, and it does not look like they are used in other files. Lets keep swagger clean

mcardosos

Cool, added some comments. Many of them are nice-to-have (if you feel inspired, you are welcome to do them in a different PR). There are also linter and model validation errors to fix :D

mcardosos · 2018-03-23T23:57:38Z

There are still errors/warnings:

jaredmoo · 2018-03-23T23:59:17Z

Yeah, these remaining warnings are all in existing files that are unrelated to this change. I don't know why they are showing up for this PR, but I won't fix them.

jaredmoo · 2018-03-24T00:01:20Z

More specifically:

Missing descriptions: in existing longTermRetention.json file that this PR did not touch.
Consider changing operation name: these are already used in SDKs so would be SDK breaking change.
Model validation: these are due to genuine service bug where these old api versions are actually not returning time zone in the datetime string. They are unrelated to this change, just showing up because I modified sql.core.json where these are used.

mcardosos · 2018-03-24T00:09:27Z

Some of them are related to this PR. Let me specify better some of them:

Model validation error. This error is on specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/capabilities.json file.
Other errors I linked specify which lines from the CI logs to take a look at.

I recommend searching for 2017-10-01-preview in the CI logs for the model validation and the linter, so you don't even look at issues from other files.

jaredmoo · 2018-03-24T23:07:42Z

PostOperationIdContainsUrlVerb for Databases_Rename ( https://travis-ci.org/Azure/azure-rest-api-specs/jobs/357637299#L847-L860 ): I would prefer to leave it as-is since it has already shipped in SDKs as Databases_Rename. I think that this warning didn't exist when this operation was originally added (in a previous api version). Moving to the new api-version is already an SDK breaking change for databases, but I think we should try to absolutely minimize the amount of breaking change. What do you think?
TrackedResourceListByResourceGroup for Databases ( https://travis-ci.org/Azure/azure-rest-api-specs/jobs/357637299#L1057-L1140 ) - we actually tried implementing this but ARM doesn't support list by resource group for tracked resources under other tracked resources.
Capabilities model parameter name warning ( https://travis-ci.org/Azure/azure-rest-api-specs/jobs/357637300#L766 ) - fixed

Thank you for your attention to detail and spotting these warnings that I had missed :)

mcardosos

Super :)

jaredmoo · 2018-03-26T01:27:04Z

Thanks so much Mariana! I am going on vacation so another one of my teammates such as @payiAzure or @nathannfan will confirm when it has reached the first production cluster and is therefore ready to merge. :)

payiAzure · 2018-03-26T17:35:25Z

@mcardosos , @jaredmoo, @nathannfan Hi Mariana, I just confirmed that our changes reached the first production cluster on Sunday, and it is ready to merge this PR.

jaredmoo added 5 commits March 21, 2018 20:56

Polish up 2017-03 databases

95999c0

Rename 2017-03 ArmSku->Sku, Status->DatabaseStatus and added OnlineSecondary CreateMode. These changes should make 2017-03 db client compatible with 2017-10 db.

Add 2017-10 databases

6aaae81

Add 2017-10 pools

901c245

Add 2017-10 capabilities

b59c08b

AutorestCI mentioned this pull request Mar 22, 2018

[AutoPR sql/resource-manager] SQL database, elastic pool, and capabilities 2017-10-01-preview Azure/azure-sdk-for-python#2251

Closed

AutorestCI mentioned this pull request Mar 22, 2018

[AutoPR sql/resource-manager] SQL database, elastic pool, and capabilities 2017-10-01-preview Azure/azure-libraries-for-java#365

Closed

This was referenced Mar 22, 2018

[AutoPR sql/resource-manager] SQL database, elastic pool, and capabilities 2017-10-01-preview Azure/azure-sdk-for-go#1394

Merged

[AutoPR sql/resource-manager] SQL database, elastic pool, and capabilities 2017-10-01-preview Azure/azure-sdk-for-node#2511

Closed

azuresdkci assigned mcardosos Mar 22, 2018

jaredmoo added 3 commits March 21, 2018 22:17

Fixed model validation warnings

cc39bca

Fixed -v3 typo

8141d3d

Set default package

9505103

mcardosos reviewed Mar 23, 2018

View reviewed changes

Fixed model validation errors by making minor example edits

6086612

mcardosos reviewed Mar 23, 2018

View reviewed changes

AutorestCI mentioned this pull request Mar 23, 2018

[AutoPR sql/resource-manager] SQL database, elastic pool, and capabilities 2017-10-01-preview Azure/azure-sdk-for-python#2272

Merged

mcardosos reviewed Mar 23, 2018

View reviewed changes

AutorestCI mentioned this pull request Mar 23, 2018

[AutoPR sql/resource-manager] SQL database, elastic pool, and capabilities 2017-10-01-preview AutorestCI/azure-libraries-for-java#8

Merged

mcardosos reviewed Mar 23, 2018

View reviewed changes

mcardosos suggested changes Mar 23, 2018

View reviewed changes

Added capabilities example and fixed delete pool example

b7dc710

mcardosos added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Mar 23, 2018

Fixed CapabilitiesList example param name

5b2ebc1

jaredmoo force-pushed the database2017 branch from 2f4cf0e to 5b2ebc1 Compare March 24, 2018 23:27

mcardosos approved these changes Mar 26, 2018

View reviewed changes

mcardosos merged commit 7920381 into Azure:master Mar 27, 2018

payiAzure mentioned this pull request Mar 28, 2018

Generate SDK.Net from swagger 'SQL database, elastic pool, and capabilities 2017-10-01-preview', and re-recording all Sql tests Azure/azure-sdk-for-net#4171

Merged

9 tasks

This was referenced Apr 10, 2018

fix Management.Sql sdk issue for customization elastic pool model Azure/azure-sdk-for-net#4206

Merged

fix customized ElasticPoolUpdate model as well Azure/azure-sdk-for-net#4209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL database, elastic pool, and capabilities 2017-10-01-preview #2734

SQL database, elastic pool, and capabilities 2017-10-01-preview #2734

jaredmoo commented Mar 22, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading

jaredmoo commented Mar 22, 2018

AutorestCI commented Mar 22, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading

jaredmoo commented Mar 22, 2018

mcardosos Mar 23, 2018

jaredmoo Mar 23, 2018

mcardosos Mar 23, 2018

jaredmoo Mar 23, 2018

mcardosos Mar 23, 2018

jaredmoo Mar 23, 2018

mcardosos Mar 23, 2018

jaredmoo Mar 23, 2018

mcardosos Mar 23, 2018

mcardosos Mar 23, 2018

mcardosos left a comment

mcardosos commented Mar 23, 2018

jaredmoo commented Mar 23, 2018

jaredmoo commented Mar 24, 2018

mcardosos commented Mar 24, 2018

jaredmoo commented Mar 24, 2018 •

edited

Loading

mcardosos left a comment

jaredmoo commented Mar 26, 2018

payiAzure commented Mar 26, 2018

SQL database, elastic pool, and capabilities 2017-10-01-preview #2734

SQL database, elastic pool, and capabilities 2017-10-01-preview #2734

Conversation

jaredmoo commented Mar 22, 2018 • edited Loading

PR information

Quality of Swagger

AutorestCI commented Mar 22, 2018 • edited Loading

Automation for azure-sdk-for-python

jaredmoo commented Mar 22, 2018

AutorestCI commented Mar 22, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented Mar 22, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Mar 22, 2018 • edited Loading

Automation for azure-sdk-for-node

jaredmoo commented Mar 22, 2018

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

mcardosos left a comment

Choose a reason for hiding this comment

mcardosos commented Mar 23, 2018

jaredmoo commented Mar 23, 2018

jaredmoo commented Mar 24, 2018

mcardosos commented Mar 24, 2018

jaredmoo commented Mar 24, 2018 • edited Loading

mcardosos left a comment

Choose a reason for hiding this comment

jaredmoo commented Mar 26, 2018

payiAzure commented Mar 26, 2018

jaredmoo commented Mar 22, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading

jaredmoo commented Mar 24, 2018 •

edited

Loading