provider/aws: Enable final snapshots for aws_rds_cluster by default #11694

stack72 · 2017-02-04T12:30:00Z

We now enable the final_snapshot of aws_rds_cluster by default. This is
a continuation of the work in #11668

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRDSCluster_takeFinalSnapshot'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/04 13:19:52 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRDSCluster_takeFinalSnapshot -timeout 120m
=== RUN   TestAccAWSRDSCluster_takeFinalSnapshot
--- PASS: TestAccAWSRDSCluster_takeFinalSnapshot (141.59s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	141.609s

radeksimko

The only real blocker is unmodified tests for aws_rds_cluster_instance, which is also why some acceptance tests are failing:
https://gist.github.com/radeksimko/5dc581dc55c90ac152ce61ef72576b27

Otherwise this looks pretty good.

radeksimko · 2017-02-09T11:20:08Z

builtin/providers/aws/resource_aws_rds_cluster_test.go

+  master_password = "mustbeeightcharaters"
+  db_cluster_parameter_group_name = "default.aurora5.6"
+  skip_final_snapshot = true
+  final_snapshot_identifier = "tf-acctest-rdscluster-snapshot-1"


I'm not sure how the namespaces work around snapshots & clusters (i.e. if each cluster has its own namespace), but I'm thinking we should randomize this, so we can run this test multiple times in parallel. What do you think?

radeksimko · 2017-02-09T11:21:14Z

builtin/providers/aws/resource_aws_rds_cluster_test.go

+				return nil
+			}
+		} else {
+			arn, err := buildRDSClusterARN(snapshot_identifier, testAccProvider.Meta().(*AWSClient).partition, testAccProvider.Meta().(*AWSClient).accountid, testAccProvider.Meta().(*AWSClient).region)


Really a nitpick, but you could reduce the length of this line by separating the cast operation onto a separate line above, i.e.

awsClient := testAccProvider.Meta().(*AWSClient)

radeksimko · 2017-02-09T11:29:45Z

builtin/providers/aws/resource_aws_rds_cluster_test.go

@@ -343,5 +438,6 @@ resource "aws_rds_cluster" "default" {
  preferred_backup_window = "03:00-09:00"
  preferred_maintenance_window = "wed:01:00-wed:01:30"
  apply_immediately = true
+  skip_final_snapshot = true


These are all great and will prevent the leakage of snapshots 👍 , but I think we have a few more in ./builtin/providers/aws/resource_aws_rds_cluster_instance_test.go - we'll either need to disable snapshots there too, or add snapshot deletion into testAccCheckAWSClusterDestroy.

e.g. https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_rds_cluster_instance_test.go#L221-L227

stack72 · 2017-02-13T13:06:49Z

@radeksimko I believe I have made the necessary changes here - I am waiting for the tests to run though :) I will post the results when they are done

stack72 · 2017-02-13T14:36:39Z

Pending the issue of the snapshot random name, all the other tests are passing:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRDSCluster'                                   ✚ ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/13 12:57:22 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRDSCluster -timeout 120m
=== RUN   TestAccAWSRDSClusterInstance_importBasic
--- PASS: TestAccAWSRDSClusterInstance_importBasic (740.18s)
=== RUN   TestAccAWSRDSCluster_importBasic
--- PASS: TestAccAWSRDSCluster_importBasic (114.17s)
=== RUN   TestAccAWSRDSClusterInstance_basic
--- PASS: TestAccAWSRDSClusterInstance_basic (1547.61s)
=== RUN   TestAccAWSRDSClusterInstance_kmsKey
--- PASS: TestAccAWSRDSClusterInstance_kmsKey (822.81s)
=== RUN   TestAccAWSRDSClusterInstance_disappears
--- PASS: TestAccAWSRDSClusterInstance_disappears (781.63s)
=== RUN   TestAccAWSRDSClusterInstance_withInstanceEnhancedMonitor
--- PASS: TestAccAWSRDSClusterInstance_withInstanceEnhancedMonitor (843.01s)
=== RUN   TestAccAWSRDSCluster_basic
--- PASS: TestAccAWSRDSCluster_basic (143.09s)
=== RUN   TestAccAWSRDSCluster_takeFinalSnapshot
--- PASS: TestAccAWSRDSCluster_takeFinalSnapshot (143.98s)
=== RUN   TestAccAWSRDSCluster_missingUserNameCausesError
--- PASS: TestAccAWSRDSCluster_missingUserNameCausesError (4.80s)
=== RUN   TestAccAWSRDSCluster_updateTags
--- PASS: TestAccAWSRDSCluster_updateTags (138.39s)
=== RUN   TestAccAWSRDSCluster_kmsKey
--- PASS: TestAccAWSRDSCluster_kmsKey (153.51s)
=== RUN   TestAccAWSRDSCluster_encrypted
--- PASS: TestAccAWSRDSCluster_encrypted (112.50s)
=== RUN   TestAccAWSRDSCluster_backupsUpdate
--- PASS: TestAccAWSRDSCluster_backupsUpdate (170.99s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	5716.689s

This has used the hard coded snapshot name

We now enable the final_snapshot of aws_rds_cluster by default. This is a continuation of the work in #11668 ``` % make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRDSCluster_takeFinalSnapshot' ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2017/02/04 13:19:52 Generated command/internal_plugin_list.go TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRDSCluster_takeFinalSnapshot -timeout 120m === RUN TestAccAWSRDSCluster_takeFinalSnapshot --- PASS: TestAccAWSRDSCluster_takeFinalSnapshot (141.59s) PASS ok github.com/hashicorp/terraform/builtin/providers/aws 141.609s ```

catsby · 2017-04-13T18:45:40Z

builtin/providers/aws/resource_aws_rds_cluster_test.go

+  master_username = "foo"
+  master_password = "mustbeeightcharaters"
+  db_cluster_parameter_group_name = "default.aurora5.6"
+  skip_final_snapshot = true


In hindsight, a testAccAWSClusterConfigWithFinalSnapshot config should probably have skip_final_snapshot = false 😜

ghost · 2020-04-14T01:55:03Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

stack72 added this to the Terraform 0.9 milestone Feb 4, 2017

stack72 added enhancement provider/aws labels Feb 4, 2017

radeksimko requested changes Feb 9, 2017

View reviewed changes

stack72 force-pushed the f-aws-rdscluster-default-snapshot branch 3 times, most recently from bcbc49b to 6290351 Compare February 13, 2017 13:05

stack72 force-pushed the f-aws-rdscluster-default-snapshot branch 2 times, most recently from 263040f to 44058a7 Compare February 13, 2017 14:27

stack72 force-pushed the f-aws-rdscluster-default-snapshot branch from 44058a7 to 93c80c0 Compare February 13, 2017 14:48

stack72 force-pushed the f-aws-rdscluster-default-snapshot branch from 93c80c0 to 43d9a45 Compare February 13, 2017 14:54

radeksimko approved these changes Feb 13, 2017

View reviewed changes

stack72 merged commit eb5bcd9 into master Feb 13, 2017

stack72 deleted the f-aws-rdscluster-default-snapshot branch February 13, 2017 19:29

catsby reviewed Apr 13, 2017

View reviewed changes

ghost locked and limited conversation to collaborators Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Enable final snapshots for aws_rds_cluster by default #11694

provider/aws: Enable final snapshots for aws_rds_cluster by default #11694

stack72 commented Feb 4, 2017

radeksimko left a comment

radeksimko Feb 9, 2017

radeksimko Feb 9, 2017

radeksimko Feb 9, 2017

radeksimko Feb 9, 2017

stack72 commented Feb 13, 2017

stack72 commented Feb 13, 2017

catsby Apr 13, 2017

ghost commented Apr 14, 2020

provider/aws: Enable final snapshots for aws_rds_cluster by default #11694

provider/aws: Enable final snapshots for aws_rds_cluster by default #11694

Conversation

stack72 commented Feb 4, 2017

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Feb 9, 2017

Choose a reason for hiding this comment

radeksimko Feb 9, 2017

Choose a reason for hiding this comment

radeksimko Feb 9, 2017

Choose a reason for hiding this comment

radeksimko Feb 9, 2017

Choose a reason for hiding this comment

stack72 commented Feb 13, 2017

stack72 commented Feb 13, 2017

catsby Apr 13, 2017

Choose a reason for hiding this comment

ghost commented Apr 14, 2020