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

[Enhancement]: dynamodb: add arg to set deletion protection for table replica #35359

Conversation

andrei-shulaev
Copy link

@andrei-shulaev andrei-shulaev commented Jan 18, 2024

Description

Adds support to enable/disable deletion protection when creating/updating a dynamodb table replica.

Relations

Closes #30213

Output from Acceptance Testing

make testacc TESTS=TestAccDynamoDBTableReplica_deletion_protection PKG=dynamodb  
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/dynamodb/... -v -count 1 -parallel 20 -run='TestAccDynamoDBTableReplica_deletion_protection'  -timeout 360m
=== RUN   TestAccDynamoDBTableReplica_deletion_protection
=== PAUSE TestAccDynamoDBTableReplica_deletion_protection
=== CONT  TestAccDynamoDBTableReplica_deletion_protection
--- PASS: TestAccDynamoDBTableReplica_deletion_protection (371.90s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   377.910s
make testacc TESTS=TestAccDynamoDBTableReplica_basic PKG=dynamodb                
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/dynamodb/... -v -count 1 -parallel 20 -run='TestAccDynamoDBTableReplica_basic'  -timeout 360m
=== RUN   TestAccDynamoDBTableReplica_basic
=== PAUSE TestAccDynamoDBTableReplica_basic
=== CONT  TestAccDynamoDBTableReplica_basic
--- PASS: TestAccDynamoDBTableReplica_basic (203.78s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   209.721s

Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/dynamodb Issues and PRs that pertain to the dynamodb service. size/M Managed by automation to categorize the size of a PR. labels Jan 18, 2024
@terraform-aws-provider terraform-aws-provider bot added the needs-triage Waiting for first response or review from a maintainer. label Jan 18, 2024
@andrei-shulaev andrei-shulaev force-pushed the f-aws_dynamodb_table_replica-add-deletion-protection branch 2 times, most recently from ad71379 to e6c9a6f Compare January 18, 2024 09:04
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @andrei-shulaev 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@andrei-shulaev andrei-shulaev marked this pull request as ready for review January 18, 2024 09:12
@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Jan 18, 2024
@andrei-shulaev andrei-shulaev changed the title feat(dynamo): add arg to set deletion protection for table replica [Enhancement]: dynamodb: add arg to set deletion protection for table replica Jan 19, 2024
@andrei-shulaev
Copy link
Author

Hey @DrFaust92, could you please take a peek?

@andrei-shulaev
Copy link
Author

@ewbankkit @justinretzolk
hey could you please have a look at this PR?

@ankon
Copy link
Contributor

ankon commented Mar 8, 2024

❤️ This would be great to have, and stop us from manually touching infrastructure!

@andrei-shulaev
Copy link
Author

@ewbankkit @justinretzolk @DrFaust92

Hey it's been a while without a review, can i get your help on it please?

@andrei-shulaev andrei-shulaev force-pushed the f-aws_dynamodb_table_replica-add-deletion-protection branch from a887fb1 to e6c9a6f Compare May 28, 2024 11:23
Copy link

Thank you for your contribution! 🚀

A new usage of AWS SDK for Go V1 was detected. Please prefer AWS SDK for Go V2 for all net-new services. If this is an enhancement or bug fix to an existing AWS SDK Go V1 based resource, this comment can be safely ignored.

For additional information refer to the AWS SDK for Go Versions page in the contributor guide.

@andrei-shulaev andrei-shulaev force-pushed the f-aws_dynamodb_table_replica-add-deletion-protection branch from e6c9a6f to b3bb3b6 Compare May 28, 2024 11:33
@andrei-shulaev andrei-shulaev force-pushed the f-aws_dynamodb_table_replica-add-deletion-protection branch from b3bb3b6 to 250de7d Compare May 28, 2024 11:43
@andrei-shulaev andrei-shulaev force-pushed the f-aws_dynamodb_table_replica-add-deletion-protection branch 2 times, most recently from 6d84a7a to cce05f7 Compare May 31, 2024 10:08
Copy link
Member

@bschaatsbergen bschaatsbergen left a comment

Choose a reason for hiding this comment

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

Hey @andrei-shulaev, thanks a lot for taking the time to raise this pull request! 👏🏼

I've left a couple drive-by comments before one of the core maintainers takes a look at this PR. Added, please make sure to update the documentation of this resource and data source with the new argument.

internal/service/dynamodb/table_replica.go Outdated Show resolved Hide resolved
internal/service/dynamodb/table_replica.go Outdated Show resolved Hide resolved
internal/service/dynamodb/table_replica.go Outdated Show resolved Hide resolved
internal/service/dynamodb/table_replica_test.go Outdated Show resolved Hide resolved
@andrei-shulaev andrei-shulaev force-pushed the f-aws_dynamodb_table_replica-add-deletion-protection branch from cce05f7 to 6f250fb Compare June 3, 2024 04:25
@bschaatsbergen bschaatsbergen requested a review from a team as a code owner November 28, 2024 10:10
@bschaatsbergen
Copy link
Member

Hey @andrei-shulaev,

Thank you for your initial work on supporting the deletion_protection_enabled argument in the table replica resource. Your analysis on the propagation delay and status reporting was spot on. I’ve made a few minor adjustments, mainly cosmetic changes and tweaks to the call order. Additionally, I added a function that introduces a 1 minute and 30 second delay (just enough for propagation to complete) during the create operation. This improves the performance of subsequent read operations (refreshes), which are more frequent than re-creating the resource. The delay is applied only when the deletion_protection_enabled argument is set.

I'm now running the tests, because they take some time I'll provide an update here once they're all ran successfully.

Thanks again for your work!

bschaatsbergen
bschaatsbergen approved these changes Nov 28, 2024
Copy link
Member

@bschaatsbergen bschaatsbergen left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 (had to run tests separately because of some strange context behavior)

 $ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_basic' -v
2024/11/28 14:44:32 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_basic
=== PAUSE TestAccDynamoDBTableReplica_basic
=== CONT  TestAccDynamoDBTableReplica_basic
--- PASS: TestAccDynamoDBTableReplica_basic (231.72s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   237.215s

 $ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_disappears' -v
2024/11/28 14:53:21 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_disappears
=== PAUSE TestAccDynamoDBTableReplica_disappears
=== CONT  TestAccDynamoDBTableReplica_disappears
--- PASS: TestAccDynamoDBTableReplica_disappears (221.18s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   226.806s

 $ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_pitrDefault' -v
2024/11/28 14:52:19 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_pitrDefault
=== PAUSE TestAccDynamoDBTableReplica_pitrDefault
=== CONT  TestAccDynamoDBTableReplica_pitrDefault
--- PASS: TestAccDynamoDBTableReplica_pitrDefault (351.79s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   357.359s

$ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_pitrKMS' -v
2024/11/28 14:53:29 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_pitrKMS
=== PAUSE TestAccDynamoDBTableReplica_pitrKMS
=== CONT  TestAccDynamoDBTableReplica_pitrKMS
--- PASS: TestAccDynamoDBTableReplica_pitrKMS (281.57s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   287.001s

 $ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_pitrDefault' -v
2024/11/28 14:52:19 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_pitrDefault
=== PAUSE TestAccDynamoDBTableReplica_pitrDefault
=== CONT  TestAccDynamoDBTableReplica_pitrDefault
--- PASS: TestAccDynamoDBTableReplica_pitrDefault (351.79s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   357.359s

 $ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_tableClass' -v
2024/11/28 15:00:08 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_tableClass
=== PAUSE TestAccDynamoDBTableReplica_tableClass
=== CONT  TestAccDynamoDBTableReplica_tableClass
--- PASS: TestAccDynamoDBTableReplica_tableClass (518.27s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   527.870s

 $ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_keys' -v
2024/11/28 15:00:04 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_keys
=== PAUSE TestAccDynamoDBTableReplica_keys
=== CONT  TestAccDynamoDBTableReplica_keys
--- PASS: TestAccDynamoDBTableReplica_keys (405.00s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   410.549s

 $ go test ./internal/service/dynamodb/... -v -count 1 -parallel 4 -run='TestAccDynamoDBTableReplica_deletionProtection' -v
2024/11/28 15:00:12 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTableReplica_deletionProtection
=== PAUSE TestAccDynamoDBTableReplica_deletionProtection
=== RUN   TestAccDynamoDBTableReplica_deletionProtectionDefault
=== PAUSE TestAccDynamoDBTableReplica_deletionProtectionDefault
=== CONT  TestAccDynamoDBTableReplica_deletionProtection
=== CONT  TestAccDynamoDBTableReplica_deletionProtectionDefault
--- PASS: TestAccDynamoDBTableReplica_deletionProtectionDefault (512.04s)
--- PASS: TestAccDynamoDBTableReplica_deletionProtection (512.90s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   525.348s

Since I’ve made some updates to the code in this PR, I’m requesting another review from the core team. Thanks for your patience!

jar-b added 7 commits December 2, 2024 11:21
This is handled outside the scope of the update handler via transparent tagging.
Changes the argument to optional/computed instead. Also consolidates acceptance tests for this argument into a single test which exercies the waiters in both the create and update methods.

```console
% make testacc PKG=dynamodb TESTS=TestAccDynamoDBTableReplica_deletionProtection
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.3 test ./internal/service/dynamodb/... -v -count 1 -parallel 20 -run='TestAccDynamoDBTableReplica_deletionProtection'  -timeout 360m
2024/12/02 15:33:27 Initializing Terraform AWS Provider...

--- PASS: TestAccDynamoDBTableReplica_deletionProtection (698.63s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   703.869s
```
Adding this argument prevents the need for a second variant which functions similarly but has a built-in delay to account for delayed propagation to the table replica.
…d` default

While this value does default to `false` when returned from AWS, the provider does not explicitly set a default value, so this descriptor should be omitted.
jar-b
jar-b previously approved these changes Dec 3, 2024
Copy link
Member

@jar-b jar-b left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

% make testacc PKG=dynamodb TESTS=TestAccDynamoDBTableReplica_
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.3 test ./internal/service/dynamodb/... -v -count 1 -parallel 20 -run='TestAccDynamoDBTableReplica_'  -timeout 360m
2024/12/03 11:14:52 Initializing Terraform AWS Provider...

--- PASS: TestAccDynamoDBTableReplica_pitr (260.06s)
=== CONT  TestAccDynamoDBTableReplica_tags_IgnoreTags_Overlap_DefaultTag
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_emptyResourceTag (266.37s)
=== CONT  TestAccDynamoDBTableReplica_basic
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyTag_OnUpdate_Replace (267.96s)
=== CONT  TestAccDynamoDBTableReplica_tags_IgnoreTags_Overlap_ResourceTag
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_updateToProviderOnly (270.02s)
=== CONT  TestAccDynamoDBTableReplica_tags_ComputedTag_OnUpdate_Add
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_nullOverlappingResourceTag (272.81s)
=== CONT  TestAccDynamoDBTableReplica_tags_ComputedTag_OnUpdate_Replace
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_nullNonOverlappingResourceTag (273.78s)
=== CONT  TestAccDynamoDBTableReplica_tags_EmptyMap
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_emptyProviderOnlyTag (275.60s)
=== CONT  TestAccDynamoDBTableReplica_tags_AddOnUpdate
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_updateToResourceOnly (276.02s)
=== CONT  TestAccDynamoDBTableReplica_tags_null
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyTag_OnCreate (277.72s)
=== CONT  TestAccDynamoDBTableReplica_tags_ComputedTag_OnCreate
--- PASS: TestAccDynamoDBTableReplica_disappears (290.68s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_overlapping (378.84s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_providerOnly (380.90s)
--- PASS: TestAccDynamoDBTableReplica_pitrKMS (383.09s)
--- PASS: TestAccDynamoDBTableReplica_pitrDefault (383.75s)
--- PASS: TestAccDynamoDBTableReplica_keys (394.85s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_nonOverlapping (402.72s)
--- PASS: TestAccDynamoDBTableReplica_tags (404.44s)
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyTag_OnUpdate_Add (412.77s)
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyMap (235.27s)
--- PASS: TestAccDynamoDBTableReplica_tags_null (234.32s)
--- PASS: TestAccDynamoDBTableReplica_tags_ComputedTag_OnUpdate_Add (242.74s)
--- PASS: TestAccDynamoDBTableReplica_tags_AddOnUpdate (237.38s)
--- PASS: TestAccDynamoDBTableReplica_tableClass (517.17s)
--- PASS: TestAccDynamoDBTableReplica_tags_ComputedTag_OnCreate (240.49s)
--- PASS: TestAccDynamoDBTableReplica_tags_IgnoreTags_Overlap_DefaultTag (258.90s)
--- PASS: TestAccDynamoDBTableReplica_tags_IgnoreTags_Overlap_ResourceTag (255.81s)
--- PASS: TestAccDynamoDBTableReplica_tags_ComputedTag_OnUpdate_Replace (253.21s)
--- PASS: TestAccDynamoDBTableReplica_basic (260.55s)
--- PASS: TestAccDynamoDBTableReplica_deletionProtection (760.28s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   766.869s

@jar-b
Copy link
Member

jar-b commented Dec 3, 2024

Thanks for your contribution, @andrei-shulaev! 👍

Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

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

Looks good! 🚀

--- PASS: TestAccDynamoDBTableReplica_disappears (211.09s)
--- PASS: TestAccDynamoDBTableReplica_pitr (246.23s)
--- PASS: TestAccDynamoDBTableReplica_tags_ComputedTag_OnUpdate_Replace (299.96s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_nullOverlappingResourceTag (301.63s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_emptyProviderOnlyTag (309.08s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_updateToProviderOnly (317.87s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_updateToResourceOnly (321.74s)
--- PASS: TestAccDynamoDBTableReplica_tags_IgnoreTags_Overlap_DefaultTag (322.95s)
--- PASS: TestAccDynamoDBTableReplica_tags_ComputedTag_OnUpdate_Add (323.12s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_emptyResourceTag (324.06s)
--- PASS: TestAccDynamoDBTableReplica_basic (326.05s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_nullNonOverlappingResourceTag (332.96s)
--- PASS: TestAccDynamoDBTableReplica_pitrKMS (355.25s)
--- PASS: TestAccDynamoDBTableReplica_pitrDefault (381.77s)
--- PASS: TestAccDynamoDBTableReplica_keys (426.55s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_overlapping (429.84s)
--- PASS: TestAccDynamoDBTableReplica_tags_IgnoreTags_Overlap_ResourceTag (431.03s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_nonOverlapping (432.09s)
--- PASS: TestAccDynamoDBTableReplica_tags (435.69s)
--- PASS: TestAccDynamoDBTableReplica_tags_ComputedTag_OnCreate (236.15s)
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyTag_OnUpdate_Add (251.82s)
--- PASS: TestAccDynamoDBTableReplica_tags_null (230.41s)
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyTag_OnCreate (345.70s)
--- PASS: TestAccDynamoDBTableReplica_tags_DefaultTags_providerOnly (313.03s)
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyMap (240.15s)
--- PASS: TestAccDynamoDBTableReplica_tags_AddOnUpdate (243.73s)
--- PASS: TestAccDynamoDBTableReplica_tags_EmptyTag_OnUpdate_Replace (271.74s)
--- PASS: TestAccDynamoDBTableReplica_deletionProtection (781.69s)
--- PASS: TestAccDynamoDBTableReplica_tableClass (475.17s)

@jar-b jar-b merged commit 0dfa71d into hashicorp:main Dec 3, 2024
41 checks passed
@github-actions github-actions bot added this to the v5.80.0 milestone Dec 3, 2024
Copy link

github-actions bot commented Dec 4, 2024

This functionality has been released in v5.80.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. Thank you!

Copy link

github-actions bot commented Jan 4, 2025

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 4, 2025
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. service/dynamodb Issues and PRs that pertain to the dynamodb service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Enable Deletion Protection for DynamoDB Table Replicas
8 participants