Feature/add snapshot schedules resource for redshift #8064

teraken0509 · 2019-03-25T09:33:01Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Changes proposed in this pull request:

Add aws_redshift_snapshot_schedule resource
Add aws_redshift_snapshot_schedule_association resource

Output from acceptance testing:

aws_redshift_snapshot_schedule

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRedshiftSnapshotSchedule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSRedshiftSnapshotSchedule_ -timeout 120m
=== RUN   TestAccAWSRedshiftSnapshotSchedule_basic
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_basic
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withDescription
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withDescription
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withTags
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withTags
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== CONT  TestAccAWSRedshiftSnapshotSchedule_basic
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withDescription
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withTags
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withDescription (34.41s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix (34.71s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_basic (53.01s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition (53.02s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withTags (54.85s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withForceDestroy (632.35s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	632.421s

aws_redshift_snapshot_schedule_association

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRedshiftSnapshotScheduleAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSRedshiftSnapshotScheduleAssociation_ -timeout 120m
=== RUN   TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== PAUSE TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== CONT  TestAccAWSRedshiftSnapshotScheduleAssociation_basic
--- PASS: TestAccAWSRedshiftSnapshotScheduleAssociation_basic (715.14s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	715.206s

teraken0509 · 2019-04-18T22:23:50Z

Rerun test after rebase master.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRedshiftSnapshotSchedule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSRedshiftSnapshotSchedule_ -timeout 120m
=== RUN   TestAccAWSRedshiftSnapshotSchedule_basic
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_basic
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withDescription
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withDescription
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withTags
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withTags
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== CONT  TestAccAWSRedshiftSnapshotSchedule_basic
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withTags
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withDescription
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withDescription (34.01s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix (34.32s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_basic (50.41s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition (50.64s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withTags (51.58s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withForceDestroy (757.58s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	757.676s

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRedshiftSnapshotScheduleAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSRedshiftSnapshotScheduleAssociation_ -timeout 120m
=== RUN   TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== PAUSE TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== CONT  TestAccAWSRedshiftSnapshotScheduleAssociation_basic
--- PASS: TestAccAWSRedshiftSnapshotScheduleAssociation_basic (656.84s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	656.917s

…t_schedule_association resources

…pshot_schedule_association resources

teraken0509 · 2019-08-03T15:45:08Z

Re-run acctest

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRedshiftSnapshotSchedule'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSRedshiftSnapshotSchedule -timeout 120m
=== RUN   TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== PAUSE TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== RUN   TestAccAWSRedshiftSnapshotSchedule_basic
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_basic
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withDescription
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withDescription
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withTags
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withTags
=== RUN   TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== PAUSE TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== CONT  TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withForceDestroy
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withTags
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withDescription
=== CONT  TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition
=== CONT  TestAccAWSRedshiftSnapshotSchedule_basic
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withDescription (36.10s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix (36.21s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_basic (53.03s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition (53.47s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withTags (54.32s)
--- PASS: TestAccAWSRedshiftSnapshotScheduleAssociation_basic (669.37s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withForceDestroy (683.99s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	684.080s

bflad

Hi @kterada0509 👋 Thanks so much for this contribution. Overall, looking really good just some minor things to fix up before we can merge this in. Please let us know if you have any questions or do not have time to implement the items.

bflad · 2019-08-06T16:10:22Z

aws/resource_aws_redshift_snapshot_schedule_association.go

+	}
+
+	var associatedCluster *redshift.ClusterAssociatedToSchedule
+	foundAssociatedCluster := false


Nit: This boolean variable is extraneous since we can check associatedCluster == nil 👍

Thanks, fix it.

bflad · 2019-08-06T16:13:32Z

aws/resource_aws_redshift_snapshot_schedule_association.go

+
+func resourceAwsRedshiftSnapshotScheduleAssociationParseId(id string) (clusterIdentifier, scheduleIdentifier string, err error) {
+	parts := strings.SplitN(id, "/", 2)
+	if len(parts) != 2 {


We should check for empty strings on either side of the / as well 👍

Suggested change

if len(parts) != 2 {

if len(parts) != 2 || parts[0] == "" || parts[1] == "" {

Thanks, fix it.

bflad · 2019-08-06T16:15:56Z

aws/resource_aws_redshift_snapshot_schedule_association.go

+
+func waitForRedshiftSnapshotScheduleAssociationActive(conn *redshift.Redshift, timeout time.Duration, clusterIdentifier, scheduleIdentifier string) error {
+	// sleep 30 seconds to give it time, until association information included to describe response
+	time.Sleep(30 * time.Second)


resource.StateChangeConf accepts a delay in its configuration so this can be moved there 😄

stateConf := &resource.StateChangeConf{ // ... Delay: 30 * time.Second, }

Thanks, fix it.

aws/resource_aws_redshift_snapshot_schedule_association_test.go

bflad · 2019-08-06T16:19:56Z

aws/resource_aws_redshift_snapshot_schedule_association_test.go

+			ClusterIdentifier:  aws.String(clusterIdentifier),
+		})
+
+		if err == nil {


We typically prefer to invert this logic so it reads a little easier:

if err != nil { return err } if resp != nil && len(resp.SnapshotSchedules) > 0 { return fmt.Errorf(/*...*/) } // no return nil since we want to check all the resources of this type

bflad · 2019-08-06T16:27:14Z

website/aws.erb

@@ -2360,6 +2360,12 @@
                                <li>
                                    <a href="/docs/providers/aws/r/redshift_snapshot_copy_grant.html">aws_redshift_snapshot_copy_grant</a>
                                </li>
+                                <li>
+                                    <a href="/docs/providers/aws/r/redshift_snapshot_schedule_association.html">redshift_snapshot_schedule_association</a>


Missing aws_ prefix 😎

Suggested change

<a href="/docs/providers/aws/r/redshift_snapshot_schedule_association.html">redshift_snapshot_schedule_association</a>

<a href="/docs/providers/aws/r/redshift_snapshot_schedule_association.html">aws_redshift_snapshot_schedule_association</a>

bflad · 2019-08-06T16:27:27Z

website/aws.erb

+                                    <a href="/docs/providers/aws/r/redshift_snapshot_schedule_association.html">redshift_snapshot_schedule_association</a>
+                                </li>
+                                <li>
+                                    <a href="/docs/providers/aws/r/redshift_snapshot_schedule.html">redshift_snapshot_schedule</a>


😎

Suggested change

<a href="/docs/providers/aws/r/redshift_snapshot_schedule.html">redshift_snapshot_schedule</a>

<a href="/docs/providers/aws/r/redshift_snapshot_schedule.html">aws_redshift_snapshot_schedule</a>

bflad · 2019-08-06T16:29:20Z

website/docs/r/redshift_snapshot_schedule.html.markdown

+* `identifier_prefix` - (Optional, Forces new resource) Creates a unique
+identifier beginning with the specified prefix. Conflicts with `identifier`.
+* `description` - (Optional) The description of the snapshot schedule.
+* `definitions` - (Optional) The definition of the snapshot schedule. The definition is made up of schedule expressions, for example "cron(30 12 *)" or "rate(12 hours)".


Nit: we might want to wrap these two examples in backticks since Markdown is interpreting the * for italics

Suggested change

* `definitions` - (Optional) The definition of the snapshot schedule. The definition is made up of schedule expressions, for example "cron(30 12 *)" or "rate(12 hours)".

* `definitions` - (Optional) The definition of the snapshot schedule. The definition is made up of schedule expressions, for example `cron(30 12 *)` or `rate(12 hours)`.

Thanks, fixed it.

bflad · 2019-08-06T16:30:20Z

website/docs/r/redshift_snapshot_schedule.html.markdown

+identifier beginning with the specified prefix. Conflicts with `identifier`.
+* `description` - (Optional) The description of the snapshot schedule.
+* `definitions` - (Optional) The definition of the snapshot schedule. The definition is made up of schedule expressions, for example "cron(30 12 *)" or "rate(12 hours)".
+* `force_destroy` - (Optional) Whether to destroy all associated settings with clusters.


Typo 😎

Suggested change

* `force_destroy` - (Optional) Whether to destroy all associated settings with clusters.

* `force_destroy` - (Optional) Whether to destroy all associated clusters with this snapshot schedule on deletion. Must be enabled and applied before attempting deletion.

bflad · 2019-08-06T16:30:46Z

website/docs/r/redshift_snapshot_schedule_association.html.markdown

+}
+
+resource "aws_redshift_snapshot_schedule_association" "default" {
+	cluster_identifier  = "${aws_redshift_cluster.default.id}"


Nit: formatting

Suggested change

cluster_identifier = "${aws_redshift_cluster.default.id}"

cluster_identifier = "${aws_redshift_cluster.default.id}"

Thanks, fixed it.

teraken0509 · 2019-08-06T22:27:22Z

Re-run acctest.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRedshiftSnapshotScheduleAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSRedshiftSnapshotScheduleAssociation_ -timeout 120m
=== RUN   TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== PAUSE TestAccAWSRedshiftSnapshotScheduleAssociation_basic
=== CONT  TestAccAWSRedshiftSnapshotScheduleAssociation_basic
--- PASS: TestAccAWSRedshiftSnapshotScheduleAssociation_basic (690.71s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	690.785s

teraken0509 · 2019-08-06T22:27:48Z

@bflad
Thanks for your review.
I'm ready for re-review.

bflad

Thank you for the updates, looks good, thanks @kterada0509 🚀

--- PASS: TestAccAWSRedshiftSnapshotSchedule_withDescription (10.08s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withIdentifierPrefix (10.12s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withMultipleDefinition (13.44s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_basic (15.01s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withTags (15.10s)
--- PASS: TestAccAWSRedshiftSnapshotSchedule_withForceDestroy (663.90s)
--- PASS: TestAccAWSRedshiftSnapshotScheduleAssociation_basic (680.42s)

ghost · 2019-08-07T17:38:01Z

This has been released in version 2.23.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2019-11-02T14:53: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. Thanks!

bflad added the new-resource Introduces a new resource. label Mar 25, 2019

teraken0509 force-pushed the feature/add-snapshot-schedules-resource-for-redshift branch from 0d115c7 to b45d731 Compare April 8, 2019 02:47

teraken0509 force-pushed the feature/add-snapshot-schedules-resource-for-redshift branch from b45d731 to aad1fd9 Compare April 18, 2019 18:13

Skaronator approved these changes Jun 13, 2019

View reviewed changes

aeschright requested a review from a team June 26, 2019 00:50

teraken0509 added 2 commits August 4, 2019 00:14

Add Snapshot Schedules Resources for Redshift

4d556fb

Add tests for aws_redshift_snapshot_schedule and aws_redshift_snapsho…

6802227

…t_schedule_association resources

teraken0509 force-pushed the feature/add-snapshot-schedules-resource-for-redshift branch 2 times, most recently from f496012 to dc84159 Compare August 3, 2019 15:19

teraken0509 added 2 commits August 4, 2019 00:21

Add documents for aws_redshift_snapshot_schedule and aws_redshift_sna…

7442fec

…pshot_schedule_association resources

fix format for type Map

a886ea1

teraken0509 force-pushed the feature/add-snapshot-schedules-resource-for-redshift branch from dc84159 to a886ea1 Compare August 3, 2019 15:22

teraken0509 added 2 commits August 4, 2019 00:25

fix typo

39be988

fix usage setTagsRedshift

9bdcec5

bflad suggested changes Aug 6, 2019

View reviewed changes

bflad self-assigned this Aug 6, 2019

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 6, 2019

teraken0509 added 6 commits August 7, 2019 07:19

Fix resource non exist check

8c2dac5

Add existant check for idparts

4f50543

Include delay setting to resource.StateChangeConf

d99b671

fix error handling for destroy check

4c47a2f

fix typo

8a111ad

Fix aws_redshift_snapshot_schedule resource attribute description

385cbc2

format example

b31d8d9

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Aug 6, 2019

bflad self-requested a review August 6, 2019 23:33

bflad added this to the v2.23.0 milestone Aug 6, 2019

bflad approved these changes Aug 7, 2019

View reviewed changes

bflad merged commit 946cf3e into hashicorp:master Aug 7, 2019

bflad added a commit that referenced this pull request Aug 7, 2019

Update CHANGELOG for #8064

8291f25

ghost locked and limited conversation to collaborators Nov 2, 2019

teraken0509 deleted the feature/add-snapshot-schedules-resource-for-redshift branch March 5, 2020 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add snapshot schedules resource for redshift #8064

Feature/add snapshot schedules resource for redshift #8064

teraken0509 commented Mar 25, 2019

teraken0509 commented Apr 18, 2019

teraken0509 commented Aug 3, 2019

bflad left a comment

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

bflad Aug 6, 2019

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

bflad Aug 6, 2019

teraken0509 Aug 6, 2019

teraken0509 commented Aug 6, 2019

teraken0509 commented Aug 6, 2019

bflad left a comment

ghost commented Aug 7, 2019

ghost commented Nov 2, 2019

	if len(parts) != 2 {
	if len(parts) != 2 \|\| parts[0] == "" \|\| parts[1] == "" {

	<a href="/docs/providers/aws/r/redshift_snapshot_schedule_association.html">redshift_snapshot_schedule_association</a>
	<a href="/docs/providers/aws/r/redshift_snapshot_schedule_association.html">aws_redshift_snapshot_schedule_association</a>

	* `definitions` - (Optional) The definition of the snapshot schedule. The definition is made up of schedule expressions, for example "cron(30 12 *)" or "rate(12 hours)".
	* `definitions` - (Optional) The definition of the snapshot schedule. The definition is made up of schedule expressions, for example `cron(30 12 *)` or `rate(12 hours)`.

	* `force_destroy` - (Optional) Whether to destroy all associated settings with clusters.
	* `force_destroy` - (Optional) Whether to destroy all associated clusters with this snapshot schedule on deletion. Must be enabled and applied before attempting deletion.

	cluster_identifier = "${aws_redshift_cluster.default.id}"
	cluster_identifier = "${aws_redshift_cluster.default.id}"

Feature/add snapshot schedules resource for redshift #8064

Feature/add snapshot schedules resource for redshift #8064

Conversation

teraken0509 commented Mar 25, 2019

Community Note

teraken0509 commented Apr 18, 2019

teraken0509 commented Aug 3, 2019

bflad 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

teraken0509 commented Aug 6, 2019

teraken0509 commented Aug 6, 2019

bflad left a comment

Choose a reason for hiding this comment

ghost commented Aug 7, 2019

ghost commented Nov 2, 2019