Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow RDS Cluster / Cluster instance to specify the engine #1591

Merged
merged 3 commits into from
Sep 21, 2017

Conversation

miromad
Copy link

@miromad miromad commented Sep 5, 2017

Making sure terraform supports Aurora PostgreSQL now that we're getting close to its GA.

  • allows explicitly setting one of two supported engine types
    aurora (default) or aurora-postgresql
  • adds optional engine_version which may be useful for pinning specific
    postgres engine version
  • maintains backward compatibility with existing aurora rds clusters
    with combinaton of defaults and optional parameters
  • used and tested under "Aurora PostgreSQL Preview" authorized aws account in us-east-1 region
github.com/terraform-providers/terraform-provider-aws$ TF_ACC=1 go test -v github.com/terraform-providers/terraform-provider-aws/aws -timeout=7200s -run "TestAccAWSRDSClusterInstance_.*"
=== RUN   TestAccAWSRDSClusterInstance_importBasic
--- PASS: TestAccAWSRDSClusterInstance_importBasic (770.69s)
=== RUN   TestAccAWSRDSClusterInstance_basic
--- PASS: TestAccAWSRDSClusterInstance_basic (1440.30s)
=== RUN   TestAccAWSRDSClusterInstance_namePrefix
--- PASS: TestAccAWSRDSClusterInstance_namePrefix (764.54s)
=== RUN   TestAccAWSRDSClusterInstance_generatedName
--- PASS: TestAccAWSRDSClusterInstance_generatedName (720.21s)
=== RUN   TestAccAWSRDSClusterInstance_kmsKey
--- PASS: TestAccAWSRDSClusterInstance_kmsKey (765.96s)
=== RUN   TestAccAWSRDSClusterInstance_disappears
--- PASS: TestAccAWSRDSClusterInstance_disappears (722.53s)
=== RUN   TestAccAWSRDSClusterInstance_withInstanceEnhancedMonitor
--- PASS: TestAccAWSRDSClusterInstance_withInstanceEnhancedMonitor (735.03s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	5919.258s

github.com/terraform-providers/terraform-provider-aws$ TF_ACC=1 go test -v github.com/terraform-providers/terraform-provider-aws/aws -timeout=7200s -run "TestAccAWSRDSCluster_.*"
=== RUN   TestAccAWSRDSCluster_importBasic
--- PASS: TestAccAWSRDSCluster_importBasic (155.95s)
=== RUN   TestAccAWSRDSCluster_basic
--- PASS: TestAccAWSRDSCluster_basic (119.60s)
=== RUN   TestAccAWSRDSCluster_namePrefix
--- PASS: TestAccAWSRDSCluster_namePrefix (185.24s)
=== RUN   TestAccAWSRDSCluster_generatedName
--- PASS: TestAccAWSRDSCluster_generatedName (196.65s)
=== RUN   TestAccAWSRDSCluster_takeFinalSnapshot
--- PASS: TestAccAWSRDSCluster_takeFinalSnapshot (197.50s)
=== RUN   TestAccAWSRDSCluster_missingUserNameCausesError
--- PASS: TestAccAWSRDSCluster_missingUserNameCausesError (9.30s)
=== RUN   TestAccAWSRDSCluster_updateTags
--- PASS: TestAccAWSRDSCluster_updateTags (175.84s)
=== RUN   TestAccAWSRDSCluster_updateIamRoles
--- PASS: TestAccAWSRDSCluster_updateIamRoles (189.80s)
=== RUN   TestAccAWSRDSCluster_kmsKey
--- PASS: TestAccAWSRDSCluster_kmsKey (195.33s)
=== RUN   TestAccAWSRDSCluster_encrypted
--- PASS: TestAccAWSRDSCluster_encrypted (152.58s)
=== RUN   TestAccAWSRDSCluster_backupsUpdate
--- PASS: TestAccAWSRDSCluster_backupsUpdate (165.99s)
=== RUN   TestAccAWSRDSCluster_iamAuth
--- PASS: TestAccAWSRDSCluster_iamAuth (141.66s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1885.459s

- allows explicitly setting one of two supported engine types
  aurora (default) or aurora-postgresql
- adds optional engine_version which may be useful for pinning specific
  postgres engine version
- maintains backward compatibility with existing aurora rds clusters
  with combinaton of defaults and optional parameters
- used and tested under "Aurora PostgreSQL Preview" authorized aws account in us-east-1 region

```
github.com/terraform-providers/terraform-provider-aws$ TF_ACC=1 go test -v github.com/terraform-providers/terraform-provider-aws/aws -timeout=7200s -run "TestAccAWSRDSClusterInstance_.*"
=== RUN   TestAccAWSRDSClusterInstance_importBasic
--- PASS: TestAccAWSRDSClusterInstance_importBasic (770.69s)
=== RUN   TestAccAWSRDSClusterInstance_basic
--- PASS: TestAccAWSRDSClusterInstance_basic (1440.30s)
=== RUN   TestAccAWSRDSClusterInstance_namePrefix
--- PASS: TestAccAWSRDSClusterInstance_namePrefix (764.54s)
=== RUN   TestAccAWSRDSClusterInstance_generatedName
--- PASS: TestAccAWSRDSClusterInstance_generatedName (720.21s)
=== RUN   TestAccAWSRDSClusterInstance_kmsKey
--- PASS: TestAccAWSRDSClusterInstance_kmsKey (765.96s)
=== RUN   TestAccAWSRDSClusterInstance_disappears
--- PASS: TestAccAWSRDSClusterInstance_disappears (722.53s)
=== RUN   TestAccAWSRDSClusterInstance_withInstanceEnhancedMonitor
--- PASS: TestAccAWSRDSClusterInstance_withInstanceEnhancedMonitor (735.03s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	5919.258s

github.com/terraform-providers/terraform-provider-aws$ TF_ACC=1 go test -v github.com/terraform-providers/terraform-provider-aws/aws -timeout=7200s -run "TestAccAWSRDSCluster_.*"
=== RUN   TestAccAWSRDSCluster_importBasic
--- PASS: TestAccAWSRDSCluster_importBasic (155.95s)
=== RUN   TestAccAWSRDSCluster_basic
--- PASS: TestAccAWSRDSCluster_basic (119.60s)
=== RUN   TestAccAWSRDSCluster_namePrefix
--- PASS: TestAccAWSRDSCluster_namePrefix (185.24s)
=== RUN   TestAccAWSRDSCluster_generatedName
--- PASS: TestAccAWSRDSCluster_generatedName (196.65s)
=== RUN   TestAccAWSRDSCluster_takeFinalSnapshot
--- PASS: TestAccAWSRDSCluster_takeFinalSnapshot (197.50s)
=== RUN   TestAccAWSRDSCluster_missingUserNameCausesError
--- PASS: TestAccAWSRDSCluster_missingUserNameCausesError (9.30s)
=== RUN   TestAccAWSRDSCluster_updateTags
--- PASS: TestAccAWSRDSCluster_updateTags (175.84s)
=== RUN   TestAccAWSRDSCluster_updateIamRoles
--- PASS: TestAccAWSRDSCluster_updateIamRoles (189.80s)
=== RUN   TestAccAWSRDSCluster_kmsKey
--- PASS: TestAccAWSRDSCluster_kmsKey (195.33s)
=== RUN   TestAccAWSRDSCluster_encrypted
--- PASS: TestAccAWSRDSCluster_encrypted (152.58s)
=== RUN   TestAccAWSRDSCluster_backupsUpdate
--- PASS: TestAccAWSRDSCluster_backupsUpdate (165.99s)
=== RUN   TestAccAWSRDSCluster_iamAuth
--- PASS: TestAccAWSRDSCluster_iamAuth (141.66s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1885.459s
```
@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 6, 2017
@jcomeaux
Copy link

jcomeaux commented Sep 9, 2017

Hi @miromad .... I believe a very similar effort has already been merged? #1415

Is the engine selection not working (e.g. setting engine = aurora-postgresql)

***also probably relevant: aws/aws-sdk-go#1470

@miromad
Copy link
Author

miromad commented Sep 11, 2017

Hi @jcomeaux.
My changes are merged with #1415. It added engine as an option to instance but not to cluster where it was left hardcoded. I also added type validation to make it more user friendly. Regarding #1470, since I tested this under preview account and everything worked no changes are needed in SDK to support it.

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @miromad

Thanks for the work here! 👍

While this looks good, just left a few comments to discuss :)

We should also update the documentation, for instance to expose that it defaults to aurora.

Optional: true,
Default: "aurora",
ForceNew: true,
ValidateFunc: validateRdsEngine,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should validate the engine value, as we would need to update the code each time a new engine is available. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point however, amazon introduced aurora for MySQL in 2014 and added PostgreSQL in 2017. Don't have insights into their product plans but so far if they keep adding one every 3 years updating code won't be an issue. I think having validation is more useful/user friendly than flexibility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :)

ValidateFunc: validateRdsEngine,
},

"engine_version": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also read the engine version around line 575

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, missed that one.

@@ -292,6 +311,7 @@ func resourceAwsRDSClusterInstanceRead(d *schema.ResourceData, meta interface{})

d.Set("publicly_accessible", db.PubliclyAccessible)
d.Set("cluster_identifier", db.DBClusterIdentifier)
d.Set("engine", db.Engine)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also read the engine_version here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix it too

@miromad
Copy link
Author

miromad commented Sep 15, 2017

Hi @Ninir,
Thanks for feedback. Will make changes (updating doc and engine_version sets in 2 places you identified) over the weekend. Let me know if anything else is needed.

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @miromad

This looks very good to me!
I just added attributes validation in the tests part:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRDSCluster_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSRDSCluster_basic -timeout 120m
=== RUN   TestAccAWSRDSCluster_basic
--- PASS: TestAccAWSRDSCluster_basic (119.98s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	120.019s

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRDSClusterInstance_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSRDSClusterInstance_basic -timeout 120m
=== RUN   TestAccAWSRDSClusterInstance_basic
--- PASS: TestAccAWSRDSClusterInstance_basic (1408.46s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1408.507s

Thank you for the work!

@Ninir Ninir merged commit be6ca29 into hashicorp:master Sep 21, 2017
@Ninir Ninir changed the title Aurora for postgres RDS engine support Allow RDS Cluster / Cluster instance to specify the engine Sep 21, 2017
nbaztec pushed a commit to nbaztec/terraform-provider-aws that referenced this pull request Sep 26, 2017
Aurora for postgres RDS engine support
@ghost
Copy link

ghost commented Sep 27, 2017

Hi @Ninir ,so does terraform supports the cluster creation in aurora postgresql

@Ninir
Copy link
Contributor

Ninir commented Sep 27, 2017

Hi @phanindra2407

It is still not globally available, so we all need to be more patient here! 😄

@jude-pieries
Copy link

Aurora-postgres is GA starting 24/10/2017 and is available in the following regions us-east-1,2 , us-west-2 and EU-west-1

@Ninir
Copy link
Contributor

Ninir commented Oct 25, 2017

Thanks for letting us know @jude-pieries :)

Here's the link: https://aws.amazon.com/fr/blogs/aws/now-available-amazon-aurora-with-postgresql-compatibility/

@atrepca
Copy link

atrepca commented Oct 26, 2017

Confirming this works now, adding engine = "aurora-postgresql" does the trick.

@ghost
Copy link

ghost commented Apr 10, 2020

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. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants