[Bug Fix] Fixes PostgreSQL collation issues #396 and #760 #1255

WodansSon · 2018-05-18T02:31:18Z

NOTE: This PR is based on the u-postgreslq branch, once the u-postgreslq branch PR #1090 is merged into master this PR will only affect the following 4 files to fix the collation issues per #396 and #760

resource_arm_postgresql_database.go
validators.go
validators_test.go
utils.go

katbyte · 2018-05-19T06:35:46Z

Hi @jeffreyCline,

Thanks for fixing this 🙂 I am going to wait review this until the other 2 PRs are merged so the diff is only the 4 file to review.

tombuildsstuff · 2018-05-23T10:09:45Z

@jeffreyCline FYI since I've merged the PostgreSQL PR I've rebased (well, merged since this was done previously) this PR so it now only contains the original changes :)

tombuildsstuff

hey @jeffreyCline

Thanks for this PR - I hope you don't mind but I've rebased/merged this since the PostgreSQL PR's been merged. I've taken a look through and left some comments inline, but this is off to a good start. If we can fix the commented issues (and the tests pass) this should be good to merge.

Thanks!

tombuildsstuff · 2018-05-23T10:15:09Z

azurerm/validators.go

+			return
+		}
+
+		if strings.Contains(v, "-") {


IMO we'd be better using a Regex match here, it appears ^[A-Za-z0-9_. ]+$ should suffice for this?

tombuildsstuff · 2018-05-23T11:29:56Z

azurerm/resource_arm_postgresql_server.go

@@ -372,6 +372,8 @@ func expandAzureRmPostgreSQLServerSku(d *schema.ResourceData) *postgresql.Sku {
 	skus := d.Get("sku").([]interface{})
 	sku := skus[0].(map[string]interface{})

+	fmt.Printf("[SKU]: %s", sku)


minor can we remove this?

DOH! Sorry, that was part of my debug code... I forgot to remove it thanks for spotting it! :)

tombuildsstuff · 2018-05-23T11:30:51Z

azurerm/resource_arm_postgresql_database.go

@@ -119,8 +120,7 @@ func resourceArmPostgreSQLDatabaseRead(d *schema.ResourceData, meta interface{})
 	d.Set("resource_group_name", resGroup)
 	d.Set("server_name", serverName)
 	d.Set("charset", resp.Charset)
-	d.Set("collation", resp.Collation)
-
+	d.Set("collation", utils.AzureRMNormalizeCollation(d.Get("collation").(string), "-", "_", -1))


providing we split this out to a separate line (to make it more readable) - we should be able to just use a string replacement directly? e.g.

collation := strings.Replace(d.Get(resp.Collation, "-", "_", -1) d.Set("collation", collation)

note: in general we shouldn't be retrieving any properties from the schema in the Read function - since they won't exist during Import and can lead to crashes.

tombuildsstuff · 2018-05-23T11:31:01Z

azurerm/utils/utils.go

+func AzureRMNormalizeCollation(input string, find string, replace string, count int) *string {
+	collation := strings.Replace(input, find, replace, count)
+	return &collation
+}


(which means we don't need this method)

…/terraform-provider-azurerm into b-postgresql

WodansSon · 2018-05-24T01:46:14Z

I made all the requested changes, I also snuck a fix in for the charset issue... PostgreSQL had the same charset casing issues that MySQL did, except opposite.... PostgreSQL always returns uppercase, while MySQL always returns lowercase… go figure. I also added two test cases to validate the casing behavior, you can see the test results below.

Test Results

Requested changes made.

tombuildsstuff

One minor crash point we should fix (which I'll push a commit for) - but this otherwise LGTM 👍

tombuildsstuff · 2018-05-24T06:24:20Z

azurerm/resource_arm_postgresql_database.go

@@ -119,7 +122,9 @@ func resourceArmPostgreSQLDatabaseRead(d *schema.ResourceData, meta interface{})
 	d.Set("resource_group_name", resGroup)
 	d.Set("server_name", serverName)
 	d.Set("charset", resp.Charset)
-	d.Set("collation", resp.Collation)
+
+	collation := strings.Replace(*resp.Collation, "-", "_", -1)


there's a potential crash here, if resp.Collation is returned nil from the API - I'll push a commit to wrap this e.g.

if collation := resp.Collation; collation != nil { v := strings.Replace(collation, "-", "_", -1) d.Set("collation", v) }

Also working around a potential crash. Tests pass: ``` $ acctests azurerm TestAccAzureRMPostgreSQLDatabase_ === RUN TestAccAzureRMPostgreSQLDatabase_importBasic --- PASS: TestAccAzureRMPostgreSQLDatabase_importBasic (402.40s) === RUN TestAccAzureRMPostgreSQLDatabase_basic --- PASS: TestAccAzureRMPostgreSQLDatabase_basic (486.39s) === RUN TestAccAzureRMPostgreSQLDatabase_charsetLowercase --- PASS: TestAccAzureRMPostgreSQLDatabase_charsetLowercase (424.71s) === RUN TestAccAzureRMPostgreSQLDatabase_charsetMixedcase --- PASS: TestAccAzureRMPostgreSQLDatabase_charsetMixedcase (424.97s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 1738.507s ```

tombuildsstuff · 2018-05-24T06:58:19Z

Tests pass after adding the crash fix / switching to use the .Properties fields (which matches the Azure response):

$ acctests azurerm TestAccAzureRMPostgreSQLDatabase_
=== RUN   TestAccAzureRMPostgreSQLDatabase_importBasic
--- PASS: TestAccAzureRMPostgreSQLDatabase_importBasic (402.40s)
=== RUN   TestAccAzureRMPostgreSQLDatabase_basic
--- PASS: TestAccAzureRMPostgreSQLDatabase_basic (486.39s)
=== RUN   TestAccAzureRMPostgreSQLDatabase_charsetLowercase
--- PASS: TestAccAzureRMPostgreSQLDatabase_charsetLowercase (424.71s)
=== RUN   TestAccAzureRMPostgreSQLDatabase_charsetMixedcase
--- PASS: TestAccAzureRMPostgreSQLDatabase_charsetMixedcase (424.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	1738.507s

ghost · 2020-03-31T14:54: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 👉 hashibot-feedback@hashicorp.com. Thanks!

WodansSon added 10 commits May 1, 2018 19:32

Update PostgreSQL to GA 2017-12-01 API

5eb4b43

Added TestCheckResourceAttr for create_mode to tests

372ab79

Merge branch 'master' into u-postgresql

20b7519

Upgraded Go SDK to v16.2.1 to match Master Branch

f34a388

Merged github conflict resolution from pull request

5554dc5

Replaced 2017-04-30-preview postgreSQL API with 2017-12-01 GA API

e3fdb64

Removed CreateMode from schema, updated tests and documentation to match

d0a8d76

Removed createmode validation from tests

546261c

Removed createmode from two hcl sections

a5ac0b6

[Bug Fix] Fixes issues #396 and #760

ae15c97

WodansSon requested review from tombuildsstuff, katbyte, mbfrahry and JunyiYi May 18, 2018 02:31

WodansSon changed the title ~~[Bug Fix] Fixes collation issues #396 and #760~~ [Bug Fix] Fixes PostgreSQL collation issues #396 and #760 May 18, 2018

katbyte added bug service/postgresql labels May 19, 2018

tombuildsstuff removed request for mbfrahry and JunyiYi May 22, 2018 20:04

WodansSon and others added 3 commits May 22, 2018 16:48

Merge branch 'master' into b-postgresql

7ab1dad

Updated validators to match master branch

931abb0

Merge branch 'master' into b-postgresql

7fbfbf1

Fixing a merge conflict

bfdf30c

tombuildsstuff previously requested changes May 23, 2018

View reviewed changes

tombuildsstuff added the waiting-response label May 23, 2018

WodansSon added 3 commits May 23, 2018 15:26

Removed debug fmt statement

08e8f36

Removed AzureRMNormalizeCollation util function

a1109b7

Updated replace methiod

fd93d56

WodansSon added 3 commits May 23, 2018 18:02

Updated validator to use regex instead of string contains

4b887bd

Made charset case insensitive and added two test cases

ab74fc2

Merge branch 'b-postgresql' of https://github.com/terraform-providers…

7855a6b

…/terraform-provider-azurerm into b-postgresql

WodansSon removed the waiting-response label May 24, 2018

tombuildsstuff approved these changes May 24, 2018

View reviewed changes

tombuildsstuff merged commit 6070152 into master May 24, 2018

tombuildsstuff deleted the b-postgresql branch May 24, 2018 07:05

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

Updating to include #1255

c759933

katbyte added this to the 1.6.0 milestone May 25, 2018

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug Fix] Fixes PostgreSQL collation issues #396 and #760 #1255

[Bug Fix] Fixes PostgreSQL collation issues #396 and #760 #1255

WodansSon commented May 18, 2018

katbyte commented May 19, 2018

tombuildsstuff commented May 23, 2018 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff May 23, 2018

tombuildsstuff May 23, 2018

WodansSon May 23, 2018

tombuildsstuff May 23, 2018

tombuildsstuff May 23, 2018

WodansSon commented May 24, 2018

tombuildsstuff left a comment

tombuildsstuff May 24, 2018

tombuildsstuff commented May 24, 2018

ghost commented Mar 31, 2020

[Bug Fix] Fixes PostgreSQL collation issues #396 and #760 #1255

[Bug Fix] Fixes PostgreSQL collation issues #396 and #760 #1255

Conversation

WodansSon commented May 18, 2018

katbyte commented May 19, 2018

tombuildsstuff commented May 23, 2018 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff May 23, 2018

Choose a reason for hiding this comment

tombuildsstuff May 23, 2018

Choose a reason for hiding this comment

WodansSon May 23, 2018

Choose a reason for hiding this comment

tombuildsstuff May 23, 2018

Choose a reason for hiding this comment

tombuildsstuff May 23, 2018

Choose a reason for hiding this comment

WodansSon commented May 24, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff May 24, 2018

Choose a reason for hiding this comment

tombuildsstuff commented May 24, 2018

ghost commented Mar 31, 2020

tombuildsstuff commented May 23, 2018 •

edited

Loading