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.

Sign up for GitHub

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

Jump to bottom

resource/aws_dynamodb_table: Refactoring #3136

Merged
merged 2 commits into from
Jan 29, 2018
Merged

resource/aws_dynamodb_table: Refactoring #3136

merged 2 commits into from
Jan 29, 2018

Conversation

radeksimko
Copy link
Member

Firstly apologies for the big diff that can be hard to read 🙈

Why

Aka what lead me to refactor this resource:

  1. Existing LOC in aws/resource_aws_dynamodb_table.go - reduced roughly by half (from 1155 to 657)
  2. Complex and deeply nested logic for updating global_secondary_index inside CRUD. This was extracted into diffDynamoDbGSI + is now tested thoroughly, which coincidentally resulted in fixing DynamoDB index non_key_attributes changes remain unrecognised #566 (acceptance test also attached to verify)
  3. Poor readability of the code making it hard to reason about it. Reduced by applying "single responsibility principle" on best-effort basis - i.e. one function does one thing - which also helped reducing the interface of many functions to bare minimum and reuse functions more.
  4. Non-standard custom retry logic w/ time.Sleep - replaced by resource.Retry & resource.StateConf per conventions
  5. We might need to modify the schema & introduce deprecations at some point to address Terraform seems not to support multiple attributes in a DynamoDB key schema #556 or No method to ignore changes in DynamoDB GSI #671 which should be much easier after this refactoring as adding backward-compatibility layers will be easier.
  6. New Resource: aws_dynamodb_table_item #1378 was (unfortunately) inspired by some of the code I'd consider non-ideal (like custom retry logic)

Test results

=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (41.27s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (41.39s)
=== RUN   TestAccAWSDynamoDbTable_importTimeToLive
--- PASS: TestAccAWSDynamoDbTable_importTimeToLive (78.83s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (79.44s)
=== RUN   TestAccAWSDynamoDbTable_ttl
--- PASS: TestAccAWSDynamoDbTable_ttl (122.97s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateCapacity
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateCapacity (128.27s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (141.56s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes (296.13s)
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (297.49s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes (606.13s)

Fixes #566

@radeksimko radeksimko added bug Addresses a defect in current functionality. service/dynamodb Issues and PRs that pertain to the dynamodb service. labels Jan 25, 2018
@radeksimko radeksimko requested a review from bflad January 25, 2018 19:37
bflad
bflad reviewed Jan 26, 2018
View reviewed changes
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.

Overall this is looking really good and is very needed. Most of my comments are nitpicks although I think the testing comments and the one about using delete() are pretty valid.

Good news is that testing passes for me locally as well at the moment:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSDynamoDb'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDynamoDb -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (55.20s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (53.65s)
=== RUN   TestAccAWSDynamoDbTable_importTimeToLive
--- PASS: TestAccAWSDynamoDbTable_importTimeToLive (53.93s)
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (380.55s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (53.85s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (53.34s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateCapacity
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateCapacity (131.84s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes (675.47s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes (312.27s)
=== RUN   TestAccAWSDynamoDbTable_ttl
--- PASS: TestAccAWSDynamoDbTable_ttl (61.38s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1831.522s

aws/resource_aws_dynamodb_table.go
)

// Number of times to retry if a throttling-related exception occurs
const DYNAMODB_MAX_THROTTLE_RETRIES = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 bye bye

aws/resource_aws_dynamodb_table.go

return resourceAwsDynamoDbTableRead(d, meta)
}
if err := waitForDynamoDbTableToBeActive(d.Id(), 10*time.Minute, conn); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: below for update we're using d.Timeout():

waitForDynamoDbTableToBeActive(d.Id(), d.Timeout(schema.TimeoutUpdate), conn)

Should we do the same here with schema.TimeoutCreate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that in the original PR but removed it to keep the PR's scope minimal, ideally no-op. But agreed otherwise.

aws/resource_aws_dynamodb_table.go Outdated
ProvisionedThroughput: throughput,
KeySchema: keyschema,
TableName: aws.String(d.Get("name").(string)),
ProvisionedThroughput: &dynamodb.ProvisionedThroughput{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: we have mixed usage of &dynamodb.ProvisionedThroughput{} and expandDynamoDbProvisionedThroughput

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch - I'll unify that all to expandDynamoDbProvisionedThroughput 👍

aws/resource_aws_dynamodb_table.go Outdated

_, err := dynamodbconn.UpdateTable(req)

ProvisionedThroughput: &dynamodb.ProvisionedThroughput{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: we have mixed usage of &dynamodb.ProvisionedThroughput{} and expandDynamoDbProvisionedThroughput

aws/resource_aws_dynamodb_table.go Outdated
if op.Create != nil {
idxName := *op.Create.IndexName
if err := waitForDynamoDbGSIToBeActive(d.Id(), idxName, conn); err != nil {
return fmt.Errorf("Error waiting for Dynamo DB GSI %q to be created: %s", idxName, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I have bad OCD. 😢 DynamoDB please

aws/structure.go Outdated
return
}

func stripCapacityAttributes(in map[string]interface{}) map[string]interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be simplified with this and also make it more future proof? (not sure if you need to copy the map first)

delete(in, "read_capacity")
delete(in, "write_capacity")

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wanted to avoid copying it, but you're right - the name suggests the function does that, so we should do it. 👌

aws/structure.go
if ttlDesc.AttributeName != nil {
m["attribute_name"] = *ttlDesc.AttributeName
if ttlDesc.TimeToLiveStatus != nil {
m["enabled"] = (*ttlDesc.TimeToLiveStatus == dynamodb.TimeToLiveStatusEnabled)
Copy link
Contributor

@bflad bflad Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if we need to consider setting this to false for nil scenario

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the field is TypeBool it's false by default, so we shouldn't need to.

aws/structure.go Outdated
d.Set("name", table.TableName)

for _, attribute := range table.KeySchema {
if *attribute.KeyType == "HASH" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: dynamodb.KeyTypeHash and dynamodb.KeyTypeRange constants are available

aws/structure.go Outdated
m := lsi.(map[string]interface{})
idxName := m["name"].(string)

// TODO: Remove this (BC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create an associated issue and add it to the comment if its important 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

#3176 Will add that to the comment.

aws/structure.go Outdated
if v, ok := data["hash_key"]; ok && v != nil && v != "" {
keySchema = append(keySchema, &dynamodb.KeySchemaElement{
AttributeName: aws.String(v.(string)),
KeyType: aws.String("HASH"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Another reminder for dynamodb.KeyTypeHash and dynamodb.KeyTypeRange 😄

@radeksimko radeksimko mentioned this pull request Jan 29, 2018
Closed
@ghost ghost added the size/XXL Managed by automation to categorize the size of a PR. label Jan 29, 2018
@radeksimko radeksimko requested a review from bflad January 29, 2018 10:53
bflad
bflad approved these changes Jan 29, 2018
View reviewed changes
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.

TC says its good to go, let's get this into the release. 👍

@radeksimko radeksimko merged commit 11745d4 into master Jan 29, 2018
@radeksimko radeksimko deleted the f-dynamodb-table-refactoring branch January 29, 2018 14:47
jocgir added a commit to coveooss/terraform-provider-aws that referenced this pull request Jan 31, 2018
@jocgir
Merge commit '8937a3a4e9d77c8089cf147861b604e3a2d8cf7e'
309f895 
* commit '8937a3a4e9d77c8089cf147861b604e3a2d8cf7e': (47 commits)
  v1.8.0
  Update CHANGELOG.md
  resource/aws_dynamodb_table: Refactoring (hashicorp#3136)
  Update CHANGELOG for hashicorp#3171
  resource/aws_elastic_beanstalk_application: Prevent crash on reading missing application
  chore(vendor): bump aws-sdk-go to v1.12.70
  typo guardduty import test
  Update CHANGELOG for hashicorp#3142
  Removed empty strings
  test/aws_dynamodb_global_table: Use single region for basic and import testing
  resource/aws_sqs_queue: Retry creation on QueueDeletedRecently for additional 10 seconds
  docs/CONTRIBUTING: Use aws_cloudwatch_dashboard for acceptance test examples
  docs/CONTRIBUTING: New Region: don't add empty mappings
  Update CHANGELOG.md
  resource/aws_rds_cluster: Retry deletion on InvalidDBClusterStateFault
  docs/CONTRIBUTING: Remove pre-split provider directory references from testacc commands
  Add service account IDs
  docs/CONTRIBUTING: New Region
  Support AWS cn-northwest-1 Ningxia (fixes hashicorp#3053)
  Update CHANGELOG.md
  ...

# Conflicts:
#	aws/validators.go
@radeksimko radeksimko mentioned this pull request Feb 1, 2018
Closed
@bflad bflad mentioned this pull request Feb 13, 2018
Merged
@bacoboy bacoboy mentioned this pull request Mar 16, 2018
Closed
@ghost
Copy link

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

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/dynamodb Issues and PRs that pertain to the dynamodb service. size/XXL Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DynamoDB index non_key_attributes changes remain unrecognised
2 participants
@radeksimko @bflad