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

Fix DynamoDB OnDemand GSI behavior #6737

Merged
merged 3 commits into from
Dec 13, 2018

Conversation

sbogacz
Copy link
Contributor

@sbogacz sbogacz commented Dec 6, 2018

allow GSIs to be created with no throughput capacity specified if they correspond to a table with PAY_PER_REQUEST billing enabled

Fixes #6736

Changes proposed in this pull request:

  • Update dynamodb_table resource schema to make read & write_capacity inputs to GSI blocks optional
  • Return an error if billing_mode is set to PROVISIONED and capacity units aren't set

I also wouldn't mind some input about how to handle the update scenario. It feels like the code should short circuit as early as possible, so maybe extracting the logic currently being used for table creation, and using it to validate the new GSI block before we compute the diff.

Thoughts?

UPDATE: I implemented the fix for updates as well, and added an acceptance test validating that the creation succeeds with a GSI.

Output from acceptance testing:

TF_ACC=1 go test -v -timeout=30m -run=TestAccAWSDynamoDbTable
=== RUN   TestAccAWSDynamoDbTableItem_basic
=== PAUSE TestAccAWSDynamoDbTableItem_basic
=== RUN   TestAccAWSDynamoDbTableItem_rangeKey
=== PAUSE TestAccAWSDynamoDbTableItem_rangeKey
=== RUN   TestAccAWSDynamoDbTableItem_withMultipleItems
=== PAUSE TestAccAWSDynamoDbTableItem_withMultipleItems
=== RUN   TestAccAWSDynamoDbTableItem_update
=== PAUSE TestAccAWSDynamoDbTableItem_update
=== RUN   TestAccAWSDynamoDbTableItem_updateWithRangeKey
=== PAUSE TestAccAWSDynamoDbTableItem_updateWithRangeKey
=== RUN   TestAccAWSDynamoDbTable_importBasic
=== PAUSE TestAccAWSDynamoDbTable_importBasic
=== RUN   TestAccAWSDynamoDbTable_importTags
=== PAUSE TestAccAWSDynamoDbTable_importTags
=== RUN   TestAccAWSDynamoDbTable_importTimeToLive
=== PAUSE TestAccAWSDynamoDbTable_importTimeToLive
=== RUN   TestAccAWSDynamoDbTable_basic
=== PAUSE TestAccAWSDynamoDbTable_basic
=== RUN   TestAccAWSDynamoDbTable_extended
=== PAUSE TestAccAWSDynamoDbTable_extended
=== RUN   TestAccAWSDynamoDbTable_enablePitr
=== PAUSE TestAccAWSDynamoDbTable_enablePitr
=== RUN   TestAccAWSDynamoDbTable_BillingMode
=== PAUSE TestAccAWSDynamoDbTable_BillingMode
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
=== PAUSE TestAccAWSDynamoDbTable_streamSpecification
=== RUN   TestAccAWSDynamoDbTable_streamSpecificationValidation
=== PAUSE TestAccAWSDynamoDbTable_streamSpecificationValidation
=== RUN   TestAccAWSDynamoDbTable_tags
=== PAUSE TestAccAWSDynamoDbTable_tags
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateCapacity
=== PAUSE TestAccAWSDynamoDbTable_gsiUpdateCapacity
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes
=== PAUSE TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes
=== PAUSE TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes
=== RUN   TestAccAWSDynamoDbTable_ttl
=== PAUSE TestAccAWSDynamoDbTable_ttl
=== RUN   TestAccAWSDynamoDbTable_attributeUpdate
=== PAUSE TestAccAWSDynamoDbTable_attributeUpdate
=== RUN   TestAccAWSDynamoDbTable_attributeUpdateValidation
=== PAUSE TestAccAWSDynamoDbTable_attributeUpdateValidation
=== RUN   TestAccAWSDynamoDbTable_encryption
=== PAUSE TestAccAWSDynamoDbTable_encryption
=== CONT  TestAccAWSDynamoDbTableItem_basic
=== CONT  TestAccAWSDynamoDbTable_streamSpecification
=== CONT  TestAccAWSDynamoDbTable_encryption
=== CONT  TestAccAWSDynamoDbTable_attributeUpdateValidation
=== CONT  TestAccAWSDynamoDbTable_attributeUpdate
=== CONT  TestAccAWSDynamoDbTable_gsiUpdateCapacity
=== CONT  TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes
=== CONT  TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_attributeUpdateValidation (3.95s)
=== CONT  TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (62.32s)
=== CONT  TestAccAWSDynamoDbTable_streamSpecificationValidation
--- PASS: TestAccAWSDynamoDbTable_streamSpecificationValidation (1.64s)
=== CONT  TestAccAWSDynamoDbTable_BillingMode
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateCapacity (107.46s)
=== CONT  TestAccAWSDynamoDbTable_ttl
--- PASS: TestAccAWSDynamoDbTable_importTags (131.60s)
=== CONT  TestAccAWSDynamoDbTable_enablePitr
--- PASS: TestAccAWSDynamoDbTable_tags (142.71s)
=== CONT  TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes
--- PASS: TestAccAWSDynamoDbTable_encryption (219.32s)
=== CONT  TestAccAWSDynamoDbTable_extended
--- PASS: TestAccAWSDynamoDbTableItem_basic (255.43s)
=== CONT  TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_ttl (149.86s)
=== CONT  TestAccAWSDynamoDbTableItem_update
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes (199.27s)
=== CONT  TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_extended (125.14s)
=== CONT  TestAccAWSDynamoDbTableItem_updateWithRangeKey
--- PASS: TestAccAWSDynamoDbTable_basic (111.65s)
=== CONT  TestAccAWSDynamoDbTableItem_withMultipleItems
--- PASS: TestAccAWSDynamoDbTableItem_update (124.02s)
=== CONT  TestAccAWSDynamoDbTable_importTimeToLive
--- PASS: TestAccAWSDynamoDbTable_enablePitr (268.90s)
=== CONT  TestAccAWSDynamoDbTableItem_rangeKey
--- PASS: TestAccAWSDynamoDbTableItem_updateWithRangeKey (120.05s)
--- PASS: TestAccAWSDynamoDbTableItem_withMultipleItems (106.17s)
--- PASS: TestAccAWSDynamoDbTable_importBasic (134.69s)
--- PASS: TestAccAWSDynamoDbTableItem_rangeKey (105.37s)
--- PASS: TestAccAWSDynamoDbTable_importTimeToLive (165.22s)
--- PASS: TestAccAWSDynamoDbTable_BillingMode (515.16s)
--- PASS: TestAccAWSDynamoDbTable_attributeUpdate (648.84s)
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes (650.99s)
PASS
ok  	github.com/sbogacz/terraform-provider-aws/aws	651.632s

…y correspond to a table with PAY_PER_REQUEST billing enabled
@ghost ghost added size/S Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/dynamodb Issues and PRs that pertain to the dynamodb service. labels Dec 6, 2018
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Dec 6, 2018
@ghost ghost added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/S Managed by automation to categorize the size of a PR. labels Dec 7, 2018
@sbogacz
Copy link
Contributor Author

sbogacz commented Dec 7, 2018

@bflad I think I'm "done" with this PR insofar that it's ready for code review. I do agree with the comments on the issue that this probably would be better categorized as a bug.

@sbogacz
Copy link
Contributor Author

sbogacz commented Dec 10, 2018

@bflad just following up to see if there's anything I can on this PR to help move it along. Thanks!

@nayanbhana
Copy link

I am having this issue too! Thanks for your work @sbogacz, hopefully someone will get this moving along

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @sbogacz 🚀

--- PASS: TestAccAWSDynamoDbTable_streamSpecificationValidation (1.64s)
--- PASS: TestAccAWSDynamoDbTable_importTags (76.22s)
--- PASS: TestAccAWSDynamoDbTable_basic (109.89s)
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (125.28s)
--- PASS: TestAccAWSDynamoDbTable_importBasic (130.26s)
--- PASS: TestAccAWSDynamoDbTable_importTimeToLive (130.35s)
--- PASS: TestAccAWSDynamoDbTable_attributeUpdateValidation (1.30s)
--- PASS: TestAccAWSDynamoDbTable_encryption (31.31s)
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateCapacity (228.50s)
--- PASS: TestAccAWSDynamoDbTable_BillingMode (249.95s)
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes (141.46s)
--- PASS: TestAccAWSDynamoDbTable_extended (253.35s)
--- PASS: TestAccAWSDynamoDbTable_ttl (141.60s)
--- PASS: TestAccAWSDynamoDbTable_enablePitr (285.30s)
--- PASS: TestAccAWSDynamoDbTable_tags (359.94s)
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes (481.43s)
--- PASS: TestAccAWSDynamoDbTable_attributeUpdate (433.90s)

@bflad bflad added this to the v1.52.0 milestone Dec 13, 2018
@bflad bflad merged commit eba7dc5 into hashicorp:master Dec 13, 2018
bflad added a commit that referenced this pull request Dec 13, 2018
@bflad
Copy link
Contributor

bflad commented Dec 13, 2018

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

@ghost
Copy link

ghost commented Apr 1, 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 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. 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.

DynamoDB On-Demand doesn't work with GSIs
3 participants