New Resources: `azurerm_postgresql_*` #210

tombuildsstuff · 2017-08-03T14:14:21Z

Adds the following resources:

azurerm_postgresql_configuration
azurerm_postgresql_database
azurerm_postgresql_firewall_rule
azurerm_postgresql_server

Each of these resources builds on one another - so unfortunately due to the way I started writing this it's ended up as one big PR - but I can split it out if needed.

Fixes #158

tombuildsstuff

picked up a few things after a fresh pair of 👀

tombuildsstuff · 2017-08-03T14:16:20Z

azurerm/resource_arm_postgresql_configuration.go

+
+	resp, err := client.Get(resGroup, serverName, name)
+	if err != nil {
+		if resp.StatusCode == http.StatusNotFound {


this can be converted too:

if responseWasNotFound(resp) {

tombuildsstuff · 2017-08-03T14:16:46Z

azurerm/resource_arm_postgresql_configuration_test.go

+
+		resp, err := client.Get(resourceGroup, serverName, name)
+		if err != nil {
+			return fmt.Errorf("Bad: Get on postgresqlConfigurationsClient: %s", err)


this should become %+v

tombuildsstuff · 2017-08-03T14:18:16Z

azurerm/resource_arm_postgresql_database.go

+
+	resp, err := client.Get(resGroup, serverName, name)
+	if err != nil {
+		if resp.StatusCode == http.StatusNotFound {


this can be updated too:

if responseWasNotFound(resp) {

tombuildsstuff · 2017-08-03T14:18:30Z

azurerm/resource_arm_postgresql_database_test.go

+
+		resp, err := client.Get(resourceGroup, serverName, name)
+		if err != nil {
+			return fmt.Errorf("Bad: Get on postgresqlDatabasesClient: %s", err)


this can be updated to %+v

tombuildsstuff · 2017-08-03T14:18:51Z

azurerm/resource_arm_postgresql_database_test.go

+
+		resp, err := client.Get(resourceGroup, serverName, name)
+
+		if err != nil {


~~this can be cleaned up, ala the other resources~~ done

tombuildsstuff · 2017-08-03T14:24:03Z

website/docs/r/postgresql_configuration.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) Specifies the name of the PostgreSQL Configration, which needs [to be a valid PostgreSQL configuration name](TODO). Changing this forces a new resource to be created.


Configration -> Configuration

this link should be https://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS

tombuildsstuff · 2017-08-03T14:27:07Z

website/docs/r/postgresql_database.html.markdown

+
+* `charset` - (Required) Specifies the Charset for the PostgreSQL Database. Changing this forces a new resource to be created.
+
+* `collation` - (Required) Specifies the Collation for the PostgreSQL Database. Changing this forces a new resource to be created.


this wants a link to the valid PostgreSQL collations

tombuildsstuff · 2017-08-03T14:27:12Z

website/docs/r/postgresql_database.html.markdown

+
+* `resource_group_name` - (Required) The name of the resource group in which the PostgreSQL Server exists. Changing this forces a new resource to be created.
+
+* `charset` - (Required) Specifies the Charset for the PostgreSQL Database. Changing this forces a new resource to be created.


this wants a link to the valid PostgreSQL charsets

tombuildsstuff · 2017-08-03T14:30:34Z

website/docs/r/postgresql_firewall_rule.html.markdown

+  version = "9.5"
+  storage_mb = "51200"
+  ssl_enforcement = "Enabled"
+}


thinking about it we could probably truncate these down

tombuildsstuff · 2017-08-03T14:30:38Z

website/docs/r/postgresql_firewall_rule.html.markdown

+  version = "9.5"
+  storage_mb = "51200"
+  ssl_enforcement = "Enabled"
+}


thinking about it we could probably truncate these down

tombuildsstuff · 2017-08-04T10:07:02Z

azurerm/resource_arm_postgresql_configuration.go

+			"value": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,


judging by the API Schema - I think we should be able to get away with a string for all of these values:

tombuildsstuff · 2017-08-19T20:35:16Z

azurerm/resource_arm_postgresql_configuration_test.go

+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+    name = "acctestRG-%d"
+    location = "West US"


need to update this to pass in the location

tombuildsstuff · 2017-08-21T07:52:04Z

Tests pass:

radeksimko

I left you comments there - many are more generic and apply to most resources - e.g. state checks in tests, detailed log messages and "name as ID" in Create functions.

Let me know what you think.

radeksimko · 2017-08-21T09:59:27Z

azurerm/resource_arm_postgresql_configuration.go

+func resourceArmPostgreSQLConfigurationCreateUpdate(d *schema.ResourceData, meta interface{}) error {
+	client := meta.(*ArmClient).postgresqlConfigurationsClient
+
+	log.Printf("[INFO] preparing arguments for AzureRM PostgreSQL Configuration creation.")


Nit: This would've been more useful on line 65 with contents of properties.

if you've got TF_LOG set to a value - you'll be seeing the http request/response anyway - so I don't think this'll add any value?

Good point, they should be the same, as long as you trust the SDK does the job. 🤷‍♂️

I just think the value of such log message is equal to "Update function of resource X reached", which is something I almost never had to debug. I remember debugging wrong/missing/additional parameters sent to the API many times though.

Admittedly I don't remember ever debugging discrepancy between SDK and actual API call (at least in AWS), so up to you - as I said it's a nitpick - so feel free to ignore. 😉

Admittedly I don't remember ever debugging discrepancy between SDK and actual API call (at least in AWS), so up to you - as I said it's a nitpick - so feel free to ignore. 😉

This should either be caught when integrating the SDK (which should now be being picked up by the SDK team) - or whilst doing new development; so I don't think that's a common scenario. As such I'm going to leave this for the moment, that said - totally happy to add this in the future if we see this is causing problems :)

radeksimko · 2017-08-21T10:03:47Z

azurerm/resource_arm_postgresql_database.go

+func resourceArmPostgreSQLDatabaseCreate(d *schema.ResourceData, meta interface{}) error {
+	client := meta.(*ArmClient).postgresqlDatabasesClient
+
+	log.Printf("[INFO] preparing arguments for AzureRM PostgreSQL Database creation.")


Nit: Similar to my other comment above.

radeksimko · 2017-08-21T10:04:27Z

azurerm/resource_arm_postgresql_database_test.go

+			{
+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMPostgreSQLDatabaseExists(resourceName),


Could we add more checks here? At least state checks via resource.TestCheckResourceAttr* would be nice.

radeksimko · 2017-08-21T10:05:12Z

azurerm/resource_arm_postgresql_configuration_test.go

+				Config: serverOnlyConfig,
+				Check: resource.ComposeTestCheckFunc(
+					// "delete" resets back to the default value
+					testCheckAzureRMPostgreSQLConfigurationValueReset(ri, "deadlock_timeout"),


Is there any particular reason you decided to implement your own check function instead of using the helper library?

Because we're not checking the local state here, but the remote API (given there's no delete available and we're resetting this back to the original value)

radeksimko · 2017-08-21T10:08:59Z

azurerm/resource_arm_postgresql_firewall_rule.go

+	if err != nil {
+		return err
+	}
+	if read.ID == nil {


😱 Is this really checking for API (mis)behaviour?

radeksimko · 2017-08-21T10:18:05Z

azurerm/resource_arm_postgresql_server_test.go

+			{
+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMPostgreSQLServerExists(resourceName),


As mentioned elsewhere - can we add some state checks here?

radeksimko · 2017-08-21T10:18:15Z

azurerm/resource_arm_postgresql_server_test.go

+			{
+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMPostgreSQLServerExists(resourceName),


As mentioned elsewhere - can we add some state checks here?

radeksimko · 2017-08-21T10:18:22Z

azurerm/resource_arm_postgresql_server_test.go

+			{
+				Config: updatedConfig,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMPostgreSQLServerExists(resourceName),


As mentioned elsewhere - can we add some state checks here?

radeksimko · 2017-08-21T10:20:40Z

azurerm/validators.go

+	return func(i interface{}, k string) (s []string, es []error) {
+		v, ok := i.(int)
+		if !ok {
+			es = append(es, fmt.Errorf("expected type of %s to be int", k))


Nitpick: It might be better to use %q as quotes can make it clearer it's a field name, not just another word. e.g. as a user if I saw expected type of key to be int I'd be tempted to think "which key?".

Good point. Updated.

radeksimko · 2017-08-21T10:22:56Z

website/docs/r/postgresql_firewall_rule.html.markdown

+}
+
+resource "azurerm_postgresql_server" "test" {
+  ...


Totally minor, but I suggest using an indented # ... for "etc" placeholders, since it co-operates better with the syntax highlighting.

radeksimko · 2017-08-21T10:26:21Z

Each of these resources builds on one another - so unfortunately due to the way I started writing this it's ended up as one big PR - but I can split it out if needed.

I understand the situation, but having a separate PR for each and another one for vendor changes would make the review a much more enjoyable process 😅

That's just for future reference.

radeksimko

https://github.com/terraform-providers/terraform-provider-azurerm/pull/210/files#diff-f7626e2a1dbd5eb89c5d808ed1e82770R257
^ that’s my only minor concern left, but not really a blocker

tombuildsstuff · 2017-09-05T15:07:06Z

Tests pass:

ghost · 2020-04-01T15:23:20Z

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!

tombuildsstuff added enhancement new-resource labels Aug 3, 2017

tombuildsstuff commented Aug 3, 2017

View reviewed changes

tombuildsstuff force-pushed the postgresql branch from a469dbb to 2519988 Compare August 3, 2017 14:44

tombuildsstuff mentioned this pull request Aug 3, 2017

Feature Request: Azure Database for PostgreSQL #158

Closed

tombuildsstuff changed the title ~~[WIP] PostgreSQL Resources~~ PostgreSQL Resources Aug 4, 2017

tombuildsstuff commented Aug 4, 2017

View reviewed changes

tombuildsstuff changed the title ~~PostgreSQL Resources~~ [WIP] PostgreSQL Resources Aug 4, 2017

tombuildsstuff force-pushed the postgresql branch from 0362e93 to 34203e3 Compare August 4, 2017 10:35

tombuildsstuff force-pushed the postgresql branch from 34203e3 to cb0806e Compare August 19, 2017 19:30

tombuildsstuff commented Aug 19, 2017

View reviewed changes

tombuildsstuff changed the title ~~[WIP] PostgreSQL Resources~~ New Resources: azurerm_postgresql_* Aug 20, 2017

tombuildsstuff requested a review from radeksimko August 21, 2017 07:52

radeksimko reviewed Aug 21, 2017

View reviewed changes

tombuildsstuff added 13 commits September 5, 2017 11:29

Adding support for PostgreSQL Servers

d8c142d

Adding support for PostgreSQL Databases

02b3713

Adding support for Firewall Rules

83f333a

Moving the intInSlice validator into the azurerm package

ce73d01

adding postgres configurations

fdc7f9e

Vendoring the Postgresql sdk

b9dcc26

Fixing rebase issues

b2eac49

Fixing the build

58946b4

Fixing the tests

2be6cdc

Fixing the vet issue

8c533f5

actually creating the configurations client

e03908f

PR comments

6bb8da0

Documentation updates

28c2e4f

tombuildsstuff added 9 commits September 5, 2017 11:37

Combining the DB + SQL sidebar navs

ae9189b

Fixing the build

f2c67d3

Test covering updating the password + ssl requirement

0a646bb

Testing additional configuration values

732ef6d

Updating the acceptance tests to check for the reset value

b2508aa

Updating the CheckDelete to check the Configuration's been deleted

dd1262d

Making the location dynamic

6b569c1

Resolving rebase issues

2f52538

Updating the tests to the new format

e9b1264

tombuildsstuff force-pushed the postgresql branch from 59f3d67 to e9b1264 Compare September 5, 2017 10:47

Adding some addiitonal resource value tests

f66404e

tombuildsstuff force-pushed the postgresql branch from 02ef31a to f66404e Compare September 5, 2017 11:45

Review fixes

5ebf58b

radeksimko approved these changes Sep 5, 2017

View reviewed changes

Splitting the SKU assignment out

d9ba935

tombuildsstuff force-pushed the postgresql branch from a25d57b to d9ba935 Compare September 5, 2017 14:07

tombuildsstuff merged commit 075b671 into master Sep 5, 2017

tombuildsstuff deleted the postgresql branch September 5, 2017 15:08

tombuildsstuff added a commit that referenced this pull request Sep 5, 2017

Detailing the PostgreSQL resources from #210

5e74f93

ghost locked and limited conversation to collaborators Apr 1, 2020


		resp, err := client.Get(resourceGroup, serverName, name)

		if err != nil {


		The following arguments are supported:

		* `name` - (Required) Specifies the name of the PostgreSQL Configration, which needs [to be a valid PostgreSQL configuration name](TODO). Changing this forces a new resource to be created.


		* `charset` - (Required) Specifies the Charset for the PostgreSQL Database. Changing this forces a new resource to be created.

		* `collation` - (Required) Specifies the Collation for the PostgreSQL Database. Changing this forces a new resource to be created.


		* `resource_group_name` - (Required) The name of the resource group in which the PostgreSQL Server exists. Changing this forces a new resource to be created.

		* `charset` - (Required) Specifies the Charset for the PostgreSQL Database. Changing this forces a new resource to be created.

New Resources: azurerm_postgresql_* #210

New Resources: azurerm_postgresql_* #210

Conversation

tombuildsstuff commented Aug 3, 2017 • 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

tombuildsstuff Aug 3, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Aug 21, 2017

radeksimko 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

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

radeksimko commented Aug 21, 2017 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Sep 5, 2017

ghost commented Apr 1, 2020

New Resources: `azurerm_postgresql_*` #210

New Resources: `azurerm_postgresql_*` #210

tombuildsstuff commented Aug 3, 2017 •

edited

Loading

tombuildsstuff Aug 3, 2017 •

edited

Loading

radeksimko commented Aug 21, 2017 •

edited

Loading