Update MySQL to GA 2017-12-01 API #1154

WodansSon · 2018-04-24T01:26:22Z

Upgrade the MySQL resource the adopt the GA API 2017-12-01. This PR will resolve issue #1004 .

tombuildsstuff · 2018-04-24T10:05:06Z

Due to an issue with the PostGreSQL API I am going to do this in two parts. First MySQL and then PostGreSQL once the API is fixed.

👍 sounds good

tombuildsstuff

hey @jeffreyCline

Thanks for opening this PR :)

I've taken a look through and left some comments inline, but this is off to a good start. If we can fix up the comments and remove the new Postgres SDK for the moment, we should be able to look into how we handle the migration path.

Thanks!

tombuildsstuff · 2018-04-24T10:06:42Z

azurerm/utils/utils.go

+
+// IntBetween returns a SchemaValidateFunc which tests if the provided value
+// is of type int and is between min and max (inclusive) and is divisible by a given number
+func IntBetweenDivisibleBy(min, max, divisor int) schema.SchemaValidateFunc {


can we make this an Internal/private function by changing the casing to intBetweenDivisibleBy. Also can we move this into validators.go since it's a validation function?

tombuildsstuff · 2018-04-24T10:07:48Z

azurerm/resource_arm_mysql_server.go

+	values := map[string]interface{}{}
+
+	values["storage_mb"] = int(*resp.StorageMB)
+	values["backup_retention_days"] = int(*resp.BackupRetentionDays)


can we add nil-checks around both of these, e.g.

if storageMB := resp.StorageMB; storageMB != nil { values["storage_mb"] = int(*storageMB) } if backupRetentionDays := resp.BackupRetentionDays; backupRetentionDays != nil { values["backup_retention_days"] = int(*backupRetentionDays) }

tombuildsstuff · 2018-04-24T10:12:46Z

azurerm/resource_arm_mysql_server.go

@@ -343,7 +403,19 @@ func flattenMySQLServerSku(d *schema.ResourceData, resp *mysql.Sku) []interface{
 	values["name"] = *resp.Name
 	values["capacity"] = int(*resp.Capacity)
 	values["tier"] = string(resp.Tier)
+	values["family"] = string(*resp.Family)


can we add nil-checks around the pointer accesses here? e.g.

if capacity := resp.Capacity; capacity != nil { values["capacity"] = *capacity }

(same for family) this helps to prevent cases where the Azure API isn't consistent

tombuildsstuff · 2018-04-24T10:18:05Z

azurerm/resource_arm_mysql_server.go

-			"storage_mb": {
-				Type:     schema.TypeInt,
+			"storage_profile": {
+				Type:     schema.TypeSet,


Since there's only one item in this List and all items are returned - we should be able to make this a List rather than a Set, which gives a better UX. Looking at the code this should be a case of changing this from schema.TypeSet -> schema.TypeList and the accessor in expandMySQLStorageProfile to:

func expandMySQLStorageProfile(d *schema.ResourceData) *mysql.StorageProfile { storageprofiles := d.Get("storage_profile").([]interface{}) storageprofile := storageprofiles[0].(map[string]interface{}) # .. }

tombuildsstuff · 2018-04-24T10:27:23Z

azurerm/resource_arm_mysql_server.go

+								"MO_Gen5_2",
+								"MO_Gen5_4",
+								"MO_Gen5_8",
+								"MO_Gen5_16",


given this varies per region and changes fairly often - I'm going to suggest that we remove this for the moment? Alternatively we could swap this out for a regex, seeing as they appear to follow the format (B|GP|MO)_Gen(\d+)_(\d+) - which looks good according to https://regex-golang.appspot.com/assets/html/index.html based on a few of the examples above?

Right, but there are specific configurations that are only supported by certain tiers, take Memory Optimized(MO) for example... MO tiers can only be Gen5 family with between 2 and 16 vCores, with the regex it would allow for invalid configurations which would fail when apply was ran. If that is acceptable I would be happy to switch over to the regex, however I was under the impression that it was not so I went this route and forced only valid combinations to be entered into the schema. Let me know what you think.

Reflecting on this - given the responses from the API can be ambiguous, I think this is probably the right approach here 👍

tombuildsstuff · 2018-04-24T14:52:39Z

website/docs/r/mysql_server.html.markdown

@@ -50,34 +57,16 @@ The following arguments are supported:

 * `sku` - (Required) A `sku` block as defined below.

+* `storage_profile` - (Required) A `storage_profile` block as defined below.
+
+* `create_mode` - (Optional) The mode to create a new server, supported values `Default` or `PointInTimeRestore`. 


The mode to create a new server, supported values Default or PointInTimeRestore.

For consistency purposes can we make this "The Creation Mode of the MySQL Server, used when restoring a database backup. Possible values include Default and PointInTimeRestore"

tombuildsstuff · 2018-04-24T14:53:18Z

azurerm/resource_arm_mysql_server.go

+				Required: false,
+				Optional: true,
+				ValidateFunc: validation.StringInSlice([]string{
+					"Default",


since this is Optional - can we add a default value for this as "Default"?

tombuildsstuff · 2018-04-24T14:54:36Z

website/docs/r/mysql_server.html.markdown

-* `name` - (Optional) Specifies the SKU Name for this MySQL Server. Possible values are: `MYSQLB50`, `MYSQLB100`, `MYSQLS100`, `MYSQLS200`, `MYSQLS400` and `MYSQLS800`.
-* `capacity` - (Optional) Specifies the DTU's for this MySQL Server. Possible values are `50` and `100` DTU's when using a `Basic` SKU and `100`, `200`, `400` or `800` when using the `Standard` SKU.
-* `tier` - (Optional) Specifies the SKU Tier for this MySQL Server. Possible values are `Basic` and `Standard`.
+* `name` - (Required) Specifies the SKU Name for this MySQL Server. See the `name` values by `tier` and `capacity` table below for valid values.


since there's a much larger set now I'm going to suggest we remove this in favour of linking to the product page for more info - as such can we remove "See the name values by tier and capacity table below for valid values."?

tombuildsstuff · 2018-04-24T14:54:57Z

website/docs/r/mysql_server.html.markdown

-* `capacity` - (Optional) Specifies the DTU's for this MySQL Server. Possible values are `50` and `100` DTU's when using a `Basic` SKU and `100`, `200`, `400` or `800` when using the `Standard` SKU.
-* `tier` - (Optional) Specifies the SKU Tier for this MySQL Server. Possible values are `Basic` and `Standard`.
+* `name` - (Required) Specifies the SKU Name for this MySQL Server. See the `name` values by `tier` and `capacity` table below for valid values.
+* `capacity` - (Required) The scale up/out capacity, representing server's compute units. Valid values depends on the `tier` of the server. See the `name` values by `tier` and `capacity` table below for valid values.


since there's a much larger set now I'm going to suggest we remove this in favour of linking to the product page for more info - as such can we remove "See the name values by tier and capacity table below for valid values."?

tombuildsstuff · 2018-04-24T14:57:47Z

website/docs/r/mysql_server.html.markdown

+`Gen5` (Intel E5-2673 v4 (Broadwell) 2.3 GHz processors).
+
+
+Suported `name` values by `tier` and `capacity`:


(as such can we remove this and the tables below)

…viders/terraform-provider-azurerm into u-mysql-postgresql

metacpp · 2018-05-02T23:45:16Z

@jeffreyCline please revert all the new files under $azure-sdk-for-go/services/postgresql/mgmt/2017-12-01/postgresql, this PR does not have any reference to this new package, it's only for MySQL.

metacpp · 2018-05-02T23:47:54Z

website/docs/r/mysql_server.html.markdown

-* `name` - (Optional) Specifies the SKU Name for this MySQL Server. Possible values are: `MYSQLB50`, `MYSQLB100`, `MYSQLS100`, `MYSQLS200`, `MYSQLS400` and `MYSQLS800`.
-* `capacity` - (Optional) Specifies the DTU's for this MySQL Server. Possible values are `50` and `100` DTU's when using a `Basic` SKU and `100`, `200`, `400` or `800` when using the `Standard` SKU.
-* `tier` - (Optional) Specifies the SKU Tier for this MySQL Server. Possible values are `Basic` and `Standard`.
+    * `name` - (Required) Specifies the SKU Name for this MySQL Server. The name of the SKU, follows the `tier` + `family` + `cores` pattern (e.g. B_Gen4_1, GP_Gen5_8). For more information see the [product documentation](https://docs.microsoft.com/en-us/rest/api/mysql/servers/create#sku).


What's the impact for old MySQL resource provisioned by old schema? Any migration work if upgrade to future new provider?

None, it is a no op for the end user. Please see the Migration Results.

Issues have been fixed.

JunyiYi · 2018-05-10T21:11:03Z

azurerm/resource_arm_mysql_server.go

@@ -42,12 +41,24 @@ func resourceArmMySqlServer() *schema.Resource {
 							Type:     schema.TypeString,
 							Required: true,
 							ValidateFunc: validation.StringInSlice([]string{


I still want to remove the duplications here because all the information are expressed in other fields already.

Maybe in a future release, right now this is a direct import of the schema.

tombuildsstuff

Hey @jeffreyCline

Apologies for the delay re-reviewing this - this mostly LGTM now, if we can remove the create_mode field for the moment (until the reset of the properties are exposed) I think we should be good to re-test this 👍

Thanks!

tombuildsstuff · 2018-05-16T13:50:26Z

azurerm/resource_arm_mysql_server.go

+								"MO_Gen5_2",
+								"MO_Gen5_4",
+								"MO_Gen5_8",
+								"MO_Gen5_16",


Reflecting on this - given the responses from the API can be ambiguous, I think this is probably the right approach here 👍

tombuildsstuff · 2018-05-16T13:52:13Z

azurerm/resource_arm_mysql_server.go

+			Version:                    mysql.ServerVersion(version),
+			SslEnforcement:             mysql.SslEnforcementEnum(sslEnforcement),
+			StorageProfile:             storageprofile,
+			CreateMode:                 mysql.CreateMode(createMode),


given we don't support the additional properties needed for importing from a Backup at this time - I think it's best we remove the schema field & default this to Default for the moment?

Removed CreateMode property and default to default.

tombuildsstuff · 2018-05-16T13:53:05Z

azurerm/resource_arm_mysql_server.go

+	values["geo_redundant_backup"] = mysql.GeoRedundantBackup(resp.GeoRedundantBackup)
+
+	storageprofile := []interface{}{values}
+	return storageprofile


minor / FYI for the future you can just return []interface{}{values} directly here :)

Fixed Issues raised by review.

tombuildsstuff

Thanks for pushing those changes - this now LGTM. I've also tested the migration path and can confirm this looks good 👍

Thanks!

Minor fixes to make the spacing consistent for sub-resources

tombuildsstuff · 2018-05-22T15:13:23Z

Pushed one minor commit to fix the spacing in the docs to match the other sub-resources - but this now LGTM and the tests pass - thanks for this!

ghost · 2020-03-31T14:57:43Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

WodansSon added 6 commits April 19, 2018 16:38

Update MySQL and PostgreSQL to support new pricing tier #1004

4c6f90d

Updated import tests to use new test name

e7357a1

Updated schema and tests

9f8f25a

Update MySQL test to ignore create_mode

4f119fb

Revert changes from PostGreSQL changes due to bug in SDK

1c9411c

Reverted PostGre to Preview API and updated MySQL docs.

2098e15

WodansSon requested review from tombuildsstuff, metacpp, mbfrahry and VaijanathB April 24, 2018 01:26

WodansSon mentioned this pull request Apr 24, 2018

ported azurerm_mysql_server to stable API #1063

Closed

Merge branch 'master' into u-mysql-postgresql

9daa7b6

tombuildsstuff added enhancement breaking-change service/mysql labels Apr 24, 2018

tombuildsstuff previously requested changes Apr 24, 2018

View reviewed changes

WodansSon added 13 commits April 24, 2018 18:44

Addressing PR comments

8c8d0bd

Merge branch 'u-mysql-postgresql' of https://github.com/terraform-pro…

7146792

…viders/terraform-provider-azurerm into u-mysql-postgresql

Fix validator test

54402d0

Fix validation test again

0ff5768

Fixed unit test for validator

8fbe2f6

Fix validation test

0689df1

Fix test

8c072fa

Fix Unit test

8f626a3

Updated expected error count

ff22c41

Display error msg in test output

45a68ab

Fix test inputs

020b284

Fix test

8f51e7f

Added test for Memory Optimized tier

11475fa

metacpp reviewed May 2, 2018

View reviewed changes

WodansSon added 2 commits May 2, 2018 17:45

removed reference to GA postgreSQL API and preview MySQL API

fa9fba9

Removing preview vendor files for MySql and GA files for PostgreSQL.

4da7790

metacpp changed the title ~~Updated MySQL to use new pricing tier~~ Update MySQL to GA 2017-12-01 API May 3, 2018

WodansSon added 3 commits May 7, 2018 15:33

Merge branch 'master' into u-mysql-postgresql

db23dd2

Upgrade Go SDK to v16.2.1 to match Master branch

e2574ed

Merge conflict resolution from PR

4096b2c

JunyiYi reviewed May 10, 2018

View reviewed changes

tombuildsstuff previously requested changes May 16, 2018

View reviewed changes

WodansSon requested a review from katbyte May 17, 2018 04:18

WodansSon added 3 commits May 17, 2018 15:57

Removed CreateMode from schema, updated tests and documentation to match

e95deb3

Missed the createmode in hcl example. DOH!

190ab58

Removed createmode from import test and more documentation hcl

7b04fa4

tombuildsstuff removed request for mbfrahry, VaijanathB and katbyte May 22, 2018 14:15

tombuildsstuff modified the milestones: Soon, 1.6.0 May 22, 2018

tombuildsstuff approved these changes May 22, 2018

View reviewed changes

Updating the indentation to match other resources

f730333

Minor fixes to make the spacing consistent for sub-resources

tombuildsstuff merged commit 7f485f0 into master May 22, 2018

tombuildsstuff deleted the u-mysql-postgresql branch May 22, 2018 16:04

tombuildsstuff added a commit that referenced this pull request May 22, 2018

Updating to include #1154

d819d59

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MySQL to GA 2017-12-01 API #1154

Update MySQL to GA 2017-12-01 API #1154

WodansSon commented Apr 24, 2018 •

edited

Loading

tombuildsstuff commented Apr 24, 2018

tombuildsstuff left a comment

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

WodansSon Apr 24, 2018 •

edited

Loading

tombuildsstuff May 16, 2018

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

tombuildsstuff Apr 24, 2018

metacpp commented May 2, 2018

metacpp May 2, 2018

WodansSon May 3, 2018

JunyiYi May 10, 2018

WodansSon May 19, 2018

tombuildsstuff left a comment

tombuildsstuff May 16, 2018

tombuildsstuff May 16, 2018

WodansSon May 19, 2018

tombuildsstuff May 16, 2018

tombuildsstuff left a comment

tombuildsstuff commented May 22, 2018

ghost commented Mar 31, 2020

		`Gen5` (Intel E5-2673 v4 (Broadwell) 2.3 GHz processors).


		Suported `name` values by `tier` and `capacity`:

Update MySQL to GA 2017-12-01 API #1154

Update MySQL to GA 2017-12-01 API #1154

Conversation

WodansSon commented Apr 24, 2018 • edited Loading

tombuildsstuff commented Apr 24, 2018

tombuildsstuff left a 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

WodansSon Apr 24, 2018 • edited Loading

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

metacpp commented May 2, 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

tombuildsstuff left a 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

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented May 22, 2018

ghost commented Mar 31, 2020

WodansSon commented Apr 24, 2018 •

edited

Loading

WodansSon Apr 24, 2018 •

edited

Loading