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

New Resource: aws_dynamodb_table_item #3238

Merged
merged 2 commits into from
Feb 9, 2018
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Feb 2, 2018

Replacement for #1378

Closes #517


This is a rough summary of my changes I added on top of @galadriel2143's PR:

  • Use standard retry helpers
  • Simplify code by extracting code into reusable functions
  • Remove fields which seemed unnecessary - if there's any interest we can add them later, but I think they'd unnecessarily increase the complexity of otherwise simple resource
  • Add validation
  • Add missing acceptance tests
  • Add missing docs

The original commit was retained, so @galadriel2143 can still receive the credit they deserve. 😉

Test results

=== RUN   TestAccAWSDynamoDbTableItem_basic
--- PASS: TestAccAWSDynamoDbTableItem_basic (41.73s)
=== RUN   TestAccAWSDynamoDbTableItem_rangeKey
--- PASS: TestAccAWSDynamoDbTableItem_rangeKey (48.55s)
=== RUN   TestAccAWSDynamoDbTableItem_withMultipleItems
--- PASS: TestAccAWSDynamoDbTableItem_withMultipleItems (56.37s)
=== RUN   TestAccAWSDynamoDbTableItem_update
--- PASS: TestAccAWSDynamoDbTableItem_update (79.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	226.544s

@radeksimko radeksimko added new-resource Introduces a new resource. service/dynamodb Issues and PRs that pertain to the dynamodb service. labels Feb 2, 2018
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 2, 2018
@radeksimko radeksimko force-pushed the f-dynamodb-table-item branch from 40d7eee to 593b86b Compare February 2, 2018 11:12
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 2, 2018
@lorengordon
Copy link
Contributor

@radeksimko any thoughts on a similar data resource? For us we would use it to read values out of dynamodb and feed them to terraform resources.

@radeksimko radeksimko force-pushed the f-dynamodb-table-item branch from 593b86b to 5ae6a9b Compare February 2, 2018 12:54
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 2, 2018
@radeksimko radeksimko force-pushed the f-dynamodb-table-item branch from 5ae6a9b to 6da6644 Compare February 2, 2018 16:55
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 2, 2018
@radeksimko
Copy link
Member Author

@lorengordon Do you mind opening a separate issue for that? We can track the interest and related discussion through it. Thanks.

@radeksimko radeksimko force-pushed the f-dynamodb-table-item branch from 6da6644 to 2453ffe Compare February 2, 2018 19:08
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 2, 2018
@radeksimko radeksimko force-pushed the f-dynamodb-table-item branch from 2453ffe to dd2ddd8 Compare February 2, 2018 19:25
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 2, 2018
@radeksimko radeksimko changed the title [WIP] New Resource: aws_dynamodb_table_item New Resource: aws_dynamodb_table_item Feb 2, 2018
@radeksimko radeksimko requested a review from bflad February 2, 2018 19:26
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 nothing blocking here. 😄 I left some notes but nothing really blocking from my perspective.

=== RUN   TestAccAWSDynamoDbTableItem_basic
--- PASS: TestAccAWSDynamoDbTableItem_basic (22.63s)
=== RUN   TestAccAWSDynamoDbTableItem_rangeKey
--- PASS: TestAccAWSDynamoDbTableItem_rangeKey (22.73s)
=== RUN   TestAccAWSDynamoDbTableItem_withMultipleItems
--- PASS: TestAccAWSDynamoDbTableItem_withMultipleItems (23.02s)
=== RUN   TestAccAWSDynamoDbTableItem_update
--- PASS: TestAccAWSDynamoDbTableItem_update (56.47s)

_, err = retryDynamoDbTableItemOperation(func() (interface{}, error) {
return conn.PutItem(&dynamodb.PutItemInput{
Item: attributes,
// Explode if item exists. We didn't create it.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 we can always add an overwrite flag later if necessary

var err error
out, err = f()
if err != nil {
if isAWSErr(err, dynamodb.ErrCodeLimitExceededException, "Subscriber limit exceeded:") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Should we move this logic up to dynamodbconn?


Provides a DynamoDB table item resource

-> **Note:** This resource is not meant to be used for managing _all_ data in your table (unless your table contains just a handful of records), it won't scale.
Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree the contents of this note are important contextually, we might want to change the wording here a little bit:

  • Changing _all_ data into large amounts of data since it is not designed to scale
  • Removing It's not supposed to serve as a backup solution either. and instead just implying this in the next sentence by saying full table **regular backups**.
  • (note: I agree with the original intent of the sentence) The last sentence is a little awkward since generally in our documentation we don't prescribe solutions for people's application/infrastructure problems in an architecture sense (unless its between competing Terraform resources). DynamoDB might be a good fit for some folks and it could be off-putting to tell them how they should be doing things. I'm not sure if there's a better way to say it, but something to consider.

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 removed the last sentence, looking back I agree it did sound a little bit biased.

if err != nil {
return err
}
expectedCount := int64(count)
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 think we can rid of this line if we just int64 the type in the signature. 😄

@radeksimko radeksimko force-pushed the f-dynamodb-table-item branch from dd2ddd8 to 495b10e Compare February 9, 2018 08:47
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 9, 2018
 - Use standard retry helpers
 - Simplify code by extracting code into reusable functions
 - Add validation
 - Add missing acceptance tests
 - Add missing docs
@radeksimko radeksimko force-pushed the f-dynamodb-table-item branch from 495b10e to 1c0871b Compare February 9, 2018 09:12
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 9, 2018
@radeksimko radeksimko merged commit e4021e5 into master Feb 9, 2018
@radeksimko radeksimko deleted the f-dynamodb-table-item branch February 9, 2018 09:19
@radeksimko radeksimko added this to the v1.9.0 milestone Feb 9, 2018
@bflad
Copy link
Contributor

bflad commented Feb 9, 2018

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@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.
Labels
new-resource Introduces a new resource. service/dynamodb Issues and PRs that pertain to the dynamodb service. size/XL Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DynamoDB item creation using a aws_dynamodb_table_item resource
4 participants