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

feat: add warm_throughput to aws_dynamodb_table #41308

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aristosvo
Copy link
Contributor

@aristosvo aristosvo commented Feb 10, 2025

Description

Adding warm_throughput to aws_dynamodb_table and the Global Secondary Index (aws_dynamodb_table. global_secondary_index)

Especially the GSI caused more code changes than I had anticipated, there is a lot of logic around create/update/recreates of the indexes.

I started with testing and I think all tests are passing , but I'm facing too much testing costs for it to run them myself for now

Relations

Closes #40141

References

Output from Acceptance Testing

# batch of GSI tests for warm throughput are going well (locally):
make testacc  TESTS=TestAccDynamoDBTable_gsiWarmThroughput_billingProvisioned PKG=dynamodb   
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/dynamodb/... -v -count 1 -parallel 20 -run='TestAccDynamoDBTable_gsiWarmThroughput_billingProvisioned'  -timeout 360m -vet=off
2025/02/11 14:00:39 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTable_gsiWarmThroughput_billingProvisioned
=== PAUSE TestAccDynamoDBTable_gsiWarmThroughput_billingProvisioned
=== CONT  TestAccDynamoDBTable_gsiWarmThroughput_billingProvisioned
--- PASS: TestAccDynamoDBTable_gsiWarmThroughput_billingProvisioned (185.35s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   190.334sterraform-provider-aws git:(f-aws_dynamodb_table-warm_troughput) ✗ make testacc  TESTS=TestAccDynamoDBTable_gsiWarmThroughput_billingPayPerRequest PKG=dynamodb
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/dynamodb/... -v -count 1 -parallel 20 -run='TestAccDynamoDBTable_gsiWarmThroughput_billingPayPerRequest'  -timeout 360m -vet=off
2025/02/11 14:08:53 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTable_gsiWarmThroughput_billingPayPerRequest
=== PAUSE TestAccDynamoDBTable_gsiWarmThroughput_billingPayPerRequest
=== CONT  TestAccDynamoDBTable_gsiWarmThroughput_billingPayPerRequest
--- PASS: TestAccDynamoDBTable_gsiWarmThroughput_billingPayPerRequest (369.36s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   374.350sterraform-provider-aws git:(f-aws_dynamodb_table-warm_troughput) ✗ make testacc  TESTS=TestAccDynamoDBTable_gsiWarmThroughput_switch PKG=dynamodb  
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/dynamodb/... -v -count 1 -parallel 20 -run='TestAccDynamoDBTable_gsiWarmThroughput_switch'  -timeout 360m -vet=off
2025/02/11 14:18:39 Initializing Terraform AWS Provider...
=== RUN   TestAccDynamoDBTable_gsiWarmThroughput_switchBilling
=== PAUSE TestAccDynamoDBTable_gsiWarmThroughput_switchBilling
=== CONT  TestAccDynamoDBTable_gsiWarmThroughput_switchBilling
--- PASS: TestAccDynamoDBTable_gsiWarmThroughput_switchBilling (973.51s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   978.548s

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. needs-triage Waiting for first response or review from a maintainer. labels Feb 10, 2025
@aristosvo aristosvo force-pushed the f-aws_dynamodb_table-warm_troughput branch from 78047a0 to 5c3aed0 Compare February 10, 2025 08:39
@aristosvo aristosvo changed the title feat: add warm_throughtput to aws_dynamodb_table feat: add warm_throughput to aws_dynamodb_table Feb 10, 2025
@acwwat
Copy link
Contributor

acwwat commented Feb 12, 2025

@aristosvo Please ensure that the aws_dynamodb_table data source is also updated. I've just fixed #41343 yesterday in which the data source code is using flatten functions from the resource code but the schema is managed separately, thus causing read errors. Thanks.

@aristosvo
Copy link
Contributor Author

@aristosvo Please ensure that the aws_dynamodb_table data source is also updated. I've just fixed #41343 yesterday in which the data source code is using flatten functions from the resource code but the schema is managed separately, thus causing read errors. Thanks.

@acwwat Thanks for the heads up! I saw your PR flying by, but didn't dig into it. Do you think we should separate the functions for data source and resource or is that not the standard way for this provider (I know that they preferred that for the Azure provider)?

@aristosvo aristosvo force-pushed the f-aws_dynamodb_table-warm_troughput branch from b678769 to e647a1e Compare February 12, 2025 19:06
@acwwat
Copy link
Contributor

acwwat commented Feb 12, 2025

@aristosvo Please ensure that the aws_dynamodb_table data source is also updated. I've just fixed #41343 yesterday in which the data source code is using flatten functions from the resource code but the schema is managed separately, thus causing read errors. Thanks.

@acwwat Thanks for the heads up! I saw your PR flying by, but didn't dig into it. Do you think we should separate the functions for data source and resource or is that not the standard way for this provider (I know that they preferred that for the Azure provider)?

@aristosvo Thanks for the message. I'll have to rely on one of the maintainers to comment on this. @justinretzolk Please see if you can chime in or loop in other maintainers for a more official recommendation? Thanks.

I am personally split, because I like code reuse especially for common logics like flattening, but the risk of causing regression is pretty high as-is. If we can lower that risk enough with more checks and guidance (e.g. if there are null checks that'd work even if schema is not updated), then my answer would be clear.

@jar-b
Copy link
Member

jar-b commented Feb 12, 2025

Whenever possible we prefer the data source match the corresponding resource schema, allowing the "flex" functions to be re-used both places. Specific to this example I think we’d want the warm_throughput block added to the data source (both as a root block and in the GSI block), along with the associated call to flattenWarmThroughput.

With Plugin Framework-based resources, AutoFlex will sort of "do the right thing" and skip attempting to set any values which are not defined in the schema. However, for legacy Plugin SDK based resources with hand written flatteners we need to keep the data source schemas more explicitly in sync.

@aristosvo aristosvo force-pushed the f-aws_dynamodb_table-warm_troughput branch from c206acb to 8739c28 Compare February 13, 2025 05:56
@aristosvo aristosvo force-pushed the f-aws_dynamodb_table-warm_troughput branch from 766ee1e to e6d9bf5 Compare February 13, 2025 11:39
@aristosvo aristosvo marked this pull request as ready for review February 13, 2025 11:42
@aristosvo aristosvo requested a review from a team as a code owner February 13, 2025 11:42
@github-actions github-actions bot added the documentation Introduces or discusses updates to documentation. label Feb 13, 2025
@aristosvo
Copy link
Contributor Author

Thanks @jar-b & @acwwat! This saved me at least one round of comments, or worse: a broken data source again :)

I really like the AutoFlex feature, but I'm afraid it might still be a heck of a job to rewrite this resource because of all the phased updates 😎

@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 Feb 13, 2025
@jar-b
Copy link
Member

jar-b commented Feb 14, 2025

I really like the AutoFlex feature, but I'm afraid it might still be a heck of a job to rewrite this resource because of all the phased updates 😎

Yes, agreed. Just to clarify, I was not requesting to migrate this resource, just describing the difference in behavior between Plugin SDK and Plugin Framework based resources. Thanks for including the data source schema changes here! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/dynamodb Issues and PRs that pertain to the dynamodb service. 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]: r/aws_dynamodb_table: Add support for warm throughput
4 participants