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

r/aws_dynamodb_table_gsi #22513

Closed
wants to merge 5 commits into from
Closed

Conversation

danquack
Copy link
Contributor

@danquack danquack commented Jan 11, 2022

Co-Authored-By: @ben-bourdin451. updated to v2 sdks from #6688

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates or Closes #17096
Relates or Closes #671

Output from acceptance testing:

$ TF_ACC=1 go test ././internal/service/dynamodb/... -v -count 1 -parallel 3  -run=TestAccAWSDynamoDbTableGsi_ -timeout 180m
=== RUN   TestAccAWSDynamoDbTableGsi_basic
=== PAUSE TestAccAWSDynamoDbTableGsi_basic
=== RUN   TestAccAWSDynamoDbTableGsi_updateCapacity
=== PAUSE TestAccAWSDynamoDbTableGsi_updateCapacity
=== RUN   TestAccAWSDynamoDbTableGsi_updateNonAttributes
=== PAUSE TestAccAWSDynamoDbTableGsi_updateNonAttributes
=== RUN   TestAccAWSDynamoDbTableGsi_delete
=== PAUSE TestAccAWSDynamoDbTableGsi_delete
=== CONT  TestAccAWSDynamoDbTableGsi_basic
=== CONT  TestAccAWSDynamoDbTableGsi_updateNonAttributes
=== CONT  TestAccAWSDynamoDbTableGsi_updateCapacity
--- PASS: TestAccAWSDynamoDbTableGsi_basic (280.10s)
=== CONT  TestAccAWSDynamoDbTableGsi_delete
--- PASS: TestAccAWSDynamoDbTableGsi_updateCapacity (309.75s)
--- PASS: TestAccAWSDynamoDbTableGsi_updateNonAttributes (691.06s)
--- PASS: TestAccAWSDynamoDbTableGsi_delete (431.54s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb   714.664s

Contributor Notes:
Open for feedback...

  • similar to aws security groups ingress/egress + security group rules, separating GSIs has an issue where attributes can be defined in different places. To avoid computed value differences when reading, I used a CustomizeDiff, but it clears the whole attribute. Not sure if you can clear just the attribute in the set in question.
  • Along the same lines, i added a feature switch to manage it externally. Is this an acceptable approach?

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. service/dynamodb Issues and PRs that pertain to the dynamodb service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/XL Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. labels Jan 11, 2022
@danquack danquack force-pushed the dynamodb-gsi branch 2 times, most recently from 693f4a9 to 63b6aaa Compare January 11, 2022 20:39
@github-actions github-actions bot added repository Repository modifications; GitHub Actions, developer docs, issue templates, codeowners, changelog. service/appmesh Issues and PRs that pertain to the appmesh service. service/chime Issues and PRs that pertain to the chime service. service/cloud9 Issues and PRs that pertain to the cloud9 service. service/cloudfront Issues and PRs that pertain to the cloudfront service. service/codebuild Issues and PRs that pertain to the codebuild service. service/devicefarm Issues and PRs that pertain to the devicefarm service. service/ec2 Issues and PRs that pertain to the ec2 service. service/ecr Issues and PRs that pertain to the ecr service. service/fsx Issues and PRs that pertain to the fsx service. service/glue Issues and PRs that pertain to the glue service. service/kafka Issues and PRs that pertain to the kafka service. service/kms Issues and PRs that pertain to the kms service. service/route53resolver Issues and PRs that pertain to the route53resolver service. service/s3control Issues and PRs that pertain to the s3control service. labels Jan 11, 2022
@github-actions github-actions bot removed repository Repository modifications; GitHub Actions, developer docs, issue templates, codeowners, changelog. service/ecr Issues and PRs that pertain to the ecr service. service/cloudfront Issues and PRs that pertain to the cloudfront service. service/kafka Issues and PRs that pertain to the kafka service. service/ec2 Issues and PRs that pertain to the ec2 service. labels Jan 11, 2022
@github-actions github-actions bot removed service/glue Issues and PRs that pertain to the glue service. service/appmesh Issues and PRs that pertain to the appmesh service. service/chime Issues and PRs that pertain to the chime service. service/route53resolver Issues and PRs that pertain to the route53resolver service. service/kms Issues and PRs that pertain to the kms service. labels Jan 11, 2022
@danquack
Copy link
Contributor Author

@ewbankkit no need to review now. This is a WIP PR and will need some love based on feedback from the GitHub issues associated

@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 11, 2022
@danquack danquack marked this pull request as draft January 17, 2022 03:45
@danquack danquack force-pushed the dynamodb-gsi branch 4 times, most recently from fd8c222 to d6b4787 Compare January 27, 2022 22:40
Co-Authored-By: Ben Bourdin <ben-bourdin451@users.noreply.github.com>
@danquack danquack changed the title WIP r/aws_dynamodb_table_gsi r/aws_dynamodb_table_gsi Jan 28, 2022
@danquack danquack marked this pull request as ready for review January 28, 2022 20:39
"global_secondary_index": {
Type: schema.TypeSet,
Optional: true,
Computed: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open for discussion, should we set the Deprecated bit?

@danquack
Copy link
Contributor Author

@ewbankkit wondering if I could bump this for your review?

@ewbankkit
Copy link
Contributor

@danquack Thanks for the work here 🎉 👏.

Our concern with attempting to decompose the DynamoDB Table resource into multiple smaller resources (as has been done for S3 Bucket recently) is that the underlying AWS API doesn't cleanly support it (unlike the S3 API which does have separate API actions for the sub-resources).
Most noticeably the attributes property is shared between the Table and any GSIs and attempting to manage this property through both an aws_dynamodb_table resource and an associated aws_dynamodb_table_gsi resource will lead to continual differences being reported by Terraform in both resources and hence to practitioner confusion.

Attempting to address this (as I see you have done) via a CustomizeDiff function on the aws_dynamodb_table is potentially brittle and a source of practitioner (and maintainer) confusion.

For example, running the GSI tests for aws_dynamodb_table now reports errors:

% make testacc TESTS=TestAccDynamoDBTable_gsi PKG=dynamodb ACCTEST_PARALLELISM=1
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/dynamodb/... -v -count 1 -parallel 1 -run='TestAccDynamoDBTable_gsi'  -timeout 180m
=== RUN   TestAccDynamoDBTable_gsiUpdateCapacity
=== PAUSE TestAccDynamoDBTable_gsiUpdateCapacity
=== RUN   TestAccDynamoDBTable_gsiUpdateOtherAttributes
=== PAUSE TestAccDynamoDBTable_gsiUpdateOtherAttributes
=== RUN   TestAccDynamoDBTable_gsiUpdateNonKeyAttributes
=== PAUSE TestAccDynamoDBTable_gsiUpdateNonKeyAttributes
=== CONT  TestAccDynamoDBTable_gsiUpdateCapacity
    table_test.go:847: Step 1/3 error: Error running apply: exit status 1
        
        Error: error creating DynamoDB Table: InvalidParameter: 1 validation error(s) found.
        - missing required field, CreateTableInput.AttributeDefinitions.
        
        
          with aws_dynamodb_table.test,
          on terraform_plugin_test.tf line 6, in resource "aws_dynamodb_table" "test":
           6: resource "aws_dynamodb_table" "test" {
        
--- FAIL: TestAccDynamoDBTable_gsiUpdateCapacity (6.15s)
=== CONT  TestAccDynamoDBTable_gsiUpdateNonKeyAttributes
    table_test.go:1044: Step 1/3 error: Error running apply: exit status 1
        
        Error: error creating DynamoDB Table: InvalidParameter: 1 validation error(s) found.
        - missing required field, CreateTableInput.AttributeDefinitions.
        
        
          with aws_dynamodb_table.test,
          on terraform_plugin_test.tf line 6, in resource "aws_dynamodb_table" "test":
           6: resource "aws_dynamodb_table" "test" {
        
--- FAIL: TestAccDynamoDBTable_gsiUpdateNonKeyAttributes (5.11s)
=== CONT  TestAccDynamoDBTable_gsiUpdateOtherAttributes
    table_test.go:915: Step 1/3 error: Error running apply: exit status 1
        
        Error: error creating DynamoDB Table: InvalidParameter: 1 validation error(s) found.
        - missing required field, CreateTableInput.AttributeDefinitions.
        
        
          with aws_dynamodb_table.test,
          on terraform_plugin_test.tf line 6, in resource "aws_dynamodb_table" "test":
           6: resource "aws_dynamodb_table" "test" {
        
--- FAIL: TestAccDynamoDBTable_gsiUpdateOtherAttributes (4.90s)
FAIL
FAIL	github.com/hashicorp/terraform-provider-aws/internal/service/dynamodb	22.120s
FAIL
make: *** [testacc] Error 1

DynamoDB tables are mission-critical resources at many practitioners’ organizations and we prefer to err on the side of caution when making significant changes to workflows built around this resource.

An AWS support ticket has been opened to start the conversation around making changes to the DynamoDB API to enable table index operations to be performed separately from the table itself, but as you can imagine this will not be a quick change.

Any limitations to the current CustomizeDiff functionality may be mitigated by eventual migration of this resource to the terraform-plugin-framework.

@github-actions
Copy link

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 Jun 18, 2022
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. provider Pertains to the provider itself, rather than any interaction with AWS. service/dynamodb Issues and PRs that pertain to the dynamodb service. size/XL 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
3 participants