New Resource: MariaDB #2406

WodansSon · 2018-11-29T00:21:56Z

Added support for the new MariaDB

fixes #2044

tombuildsstuff

hey @jeffreyCline

I've taken a look through and left some minor comments in line but on the whole this looks pretty good - if we can fix those up this otherwise should be good to merge 👍

Thanks!

tombuildsstuff · 2018-11-29T09:27:13Z

azurerm/resource_arm_mariadb_server.go

+								"MO_Gen5_8",
+								"MO_Gen5_16",
+							}, true),
+							DiffSuppressFunc: suppress.CaseDifference,


since this is a new resource can we make this case sensitive?

tombuildsstuff · 2018-11-29T09:27:32Z

azurerm/resource_arm_mariadb_server.go

+								string(mariadb.GeneralPurpose),
+								string(mariadb.MemoryOptimized),
+							}, true),
+							DiffSuppressFunc: suppress.CaseDifference,


(same here) can we make this case sensitive?

tombuildsstuff · 2018-11-29T09:29:17Z

azurerm/resource_arm_mariadb_server.go

+								string(mariadb.Enabled),
+								string(mariadb.Disabled),
+							}, true),
+							DiffSuppressFunc: suppress.CaseDifference,


(same here)

tombuildsstuff · 2018-11-29T09:29:31Z

azurerm/resource_arm_mariadb_server.go

+					string(mariadb.SslEnforcementEnumDisabled),
+					string(mariadb.SslEnforcementEnumEnabled),
+				}, true),
+				DiffSuppressFunc: suppress.CaseDifference,


tombuildsstuff · 2018-11-29T09:31:11Z

azurerm/resource_arm_mariadb_server.go

+			tier, _ := diff.GetOk("sku.0.tier")
+			storageMB, _ := diff.GetOk("storage_profile.0.storage_mb")
+
+			if strings.ToLower(tier.(string)) == "basic" && storageMB.(int) > 1048576 {


both of these fields are likely to be interpolated, as such it's likely the values won't be resolved at this point (due to a known bug in customizeDiff) and these values would be nil, so there'd be a crash here - as such I think we'd be better to put this logic in the Create method for the moment, unfortunately

Thanks for the heads up about customizeDiff, moved the validation to the create method.

website/docs/r/mariadb_server.html.markdown

tombuildsstuff · 2018-11-29T09:42:03Z

website/docs/r/mariadb_server.html.markdown

+
+---
+
+`sku` supports the following:


in some of the newer resources this has been updated to:

Suggested change

`sku` supports the following:

A `sku` block supports the following:

website/docs/r/mariadb_server.html.markdown

tombuildsstuff · 2018-11-29T09:42:42Z

website/docs/r/mariadb_server.html.markdown

+
+# azurerm_mariadb_server
+
+Manage a MariaDB Server.


Suggested change

Manage a MariaDB Server.

Manages a MariaDB Server.

website/docs/r/mariadb_server.html.markdown

Co-Authored-By: jeffreyCline <[email protected]>

katbyte

Hi @jeffreyCline,

Thank you for the changes, i've taken a look through and left some mostly minor and stylistic comments.

The major issue is not accessing the response properties through the .Properties object after a nil check.

website/docs/r/mariadb_server.html.markdown

azurerm/resource_arm_mariadb_server.go

katbyte · 2018-11-30T18:47:54Z

azurerm/resource_arm_mariadb_server.go

+	}
+
+	err = future.WaitForCompletionRef(ctx, client.Client)
+	if err != nil {


Very minor, but these two lines could be merged too.

Updated both.

katbyte · 2018-11-30T18:48:52Z

azurerm/resource_arm_mariadb_server.go

+	}
+}
+
+func flattenMariaDbServerSku(resp *mariadb.Sku) []interface{} {


minor, but should be be sku instead of resp?

katbyte · 2018-11-30T18:49:57Z

azurerm/resource_arm_mariadb_server.go

+	return []interface{}{values}
+}
+
+func flattenMariaDbStorageProfile(resp *mariadb.StorageProfile) []interface{} {


Same here, should this be storage or profile rather then resp?

katbyte · 2018-11-30T18:56:41Z

azurerm/resource_arm_mariadb_server_test.go

+
+//
+
+func testCheckAzureRMMariaDbServerExists(name string) resource.TestCheckFunc {


Could we rename name to resourceName to prevent the shadowing down below on line 245?

Yeah, good catch... didn't see that 😄

azurerm/resource_arm_mariadb_server_test.go

Co-Authored-By: jeffreyCline <[email protected]>

katbyte

@jeffreyCline,

Thanks for the updates, aside from one comment left inline LGTM 👍

azurerm/resource_arm_mariadb_server.go

+	}
+
+	if properties := resp.ServerProperties; properties != nil {
+		d.Set("administrator_login", properties.AdministratorLogin)


…erraform-provider-azurerm into nr-mariadb

…form-provider-azurerm into nr-mariadb

…erraform-provider-azurerm into nr-mariadb

katbyte · 2018-12-04T02:08:37Z

LGTM 💯

Feel free to merge

WodansSon · 2018-12-04T02:37:57Z

ghost · 2019-03-05T14:30:59Z

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 👉 [email protected]. Thanks!

MariaDB Support

db2dddd

WodansSon requested review from tombuildsstuff, katbyte, metacpp and JunyiYi November 29, 2018 00:21

ghost added the size/XXL label Nov 29, 2018

Update for lint error

1c8ff0e

WodansSon added this to the 1.20.0 milestone Nov 29, 2018

WodansSon added new-resource service/mariadb labels Nov 29, 2018

tombuildsstuff reviewed Nov 29, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Nov 29, 2018

tombuildsstuff and others added 10 commits November 29, 2018 11:45

Update website/docs/r/mariadb_server.html.markdown

7bcfdff

Co-Authored-By: jeffreyCline <[email protected]>

Update website/docs/r/mariadb_server.html.markdown

98a1da6

Co-Authored-By: jeffreyCline <[email protected]>

Updates from PR comments

6003c57

Update azurerm/resource_arm_mariadb_server.go

79a8655

Co-Authored-By: jeffreyCline <[email protected]>

Update website/docs/r/mariadb_server.html.markdown

096fdab

Co-Authored-By: jeffreyCline <[email protected]>

Update website/docs/r/mariadb_server.html.markdown

1bad186

Co-Authored-By: jeffreyCline <[email protected]>

Update website/docs/r/mariadb_server.html.markdown

88b84cd

Co-Authored-By: jeffreyCline <[email protected]>

Fix lint error

4d8fcd9

Fix lint error

7f6efaf

Updated docs and added vCore validation

3f05b1b

WodansSon removed the waiting-response label Nov 29, 2018

katbyte requested changes Nov 30, 2018

View reviewed changes

katbyte added the waiting-response label Nov 30, 2018

katbyte and others added 5 commits November 30, 2018 11:30

Update website/docs/r/mariadb_server.html.markdown

013c3ec

Co-Authored-By: jeffreyCline <[email protected]>

Update azurerm/resource_arm_mariadb_server.go

2f1868d

Co-Authored-By: jeffreyCline <[email protected]>

Update azurerm/resource_arm_mariadb_server.go

905d304

Co-Authored-By: jeffreyCline <[email protected]>

Update azurerm/resource_arm_mariadb_server_test.go

51da5fc

Co-Authored-By: jeffreyCline <[email protected]>

Update azurerm/resource_arm_mariadb_server.go

2f95967

Co-Authored-By: jeffreyCline <[email protected]>

katbyte and others added 5 commits November 30, 2018 11:32

Update azurerm/resource_arm_mariadb_server.go

cef2387

Co-Authored-By: jeffreyCline <[email protected]>

Update azurerm/resource_arm_mariadb_server.go

38de0b7

Co-Authored-By: jeffreyCline <[email protected]>

Update azurerm/resource_arm_mariadb_server.go

f80a39e

Co-Authored-By: jeffreyCline <[email protected]>

Update azurerm/resource_arm_mariadb_server.go

384ae16

Co-Authored-By: jeffreyCline <[email protected]>

Updates per PR comments add more name validation

e941a64

WodansSon removed the waiting-response label Nov 30, 2018

WodansSon added 2 commits November 30, 2018 15:08

Fixed lint errors

d2d56e5

Fixed lint error

5a4c955

katbyte approved these changes Dec 4, 2018

View reviewed changes

azurerm/resource_arm_mariadb_server.go

}

if properties := resp.ServerProperties; properties != nil {

d.Set("administrator_login", properties.AdministratorLogin)

This comment was marked as outdated.

Sign in to view

WodansSon added 8 commits December 3, 2018 17:12

Updated changelog and PR comment

a3a7958

Updated per PR comment

b4985da

Merge branch 'nr-mariadb' of https://github.com/terraform-providers/t…

bd165b4

…erraform-provider-azurerm into nr-mariadb

Merge branch 'master' of https://github.com/terraform-providers/terra…

1472d03

…form-provider-azurerm into nr-mariadb

Merge branch 'master' into nr-mariadb

751b40b

Sync changlog with master

d510f24

Merge branch 'nr-mariadb' of https://github.com/terraform-providers/t…

e5da692

…erraform-provider-azurerm into nr-mariadb

Update changelog with mariadb

f86813c

Fixed one test issue

fbd9a18

WodansSon merged commit 378109b into master Dec 4, 2018

WodansSon deleted the nr-mariadb branch December 4, 2018 02:38

ghost locked and limited conversation to collaborators Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: MariaDB #2406

New Resource: MariaDB #2406

WodansSon commented Nov 29, 2018 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Nov 29, 2018

WodansSon Nov 29, 2018

tombuildsstuff Nov 29, 2018

WodansSon Nov 29, 2018

tombuildsstuff Nov 29, 2018

WodansSon Nov 29, 2018

tombuildsstuff Nov 29, 2018

WodansSon Nov 29, 2018

tombuildsstuff Nov 29, 2018

WodansSon Nov 29, 2018

tombuildsstuff Nov 29, 2018

tombuildsstuff Nov 29, 2018

katbyte left a comment

katbyte Nov 30, 2018

WodansSon Nov 30, 2018

katbyte Nov 30, 2018

WodansSon Nov 30, 2018

katbyte Nov 30, 2018

WodansSon Nov 30, 2018

katbyte Nov 30, 2018

WodansSon Nov 30, 2018

katbyte left a comment

This comment was marked as outdated.

katbyte commented Dec 4, 2018

WodansSon commented Dec 4, 2018

ghost commented Mar 5, 2019

	`sku` supports the following:
	A `sku` block supports the following:


		//

		func testCheckAzureRMMariaDbServerExists(name string) resource.TestCheckFunc {

New Resource: MariaDB #2406

New Resource: MariaDB #2406

Conversation

WodansSon commented Nov 29, 2018 • edited Loading

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

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

katbyte 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

katbyte commented Dec 4, 2018

WodansSon commented Dec 4, 2018

ghost commented Mar 5, 2019

WodansSon commented Nov 29, 2018 •

edited

Loading