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

provider/aws: aws_dynamodb_table Add support for TimeToLive #14104

Merged
merged 3 commits into from
May 1, 2017
Merged

provider/aws: aws_dynamodb_table Add support for TimeToLive #14104

merged 3 commits into from
May 1, 2017
+254 −0

Conversation

rlweb
Copy link
Contributor

@rlweb rlweb commented Apr 30, 2017

This addresses #12530

I'm a bit of a noob at Go (done the whole tutorial + making a few things) and the whole open source thing, so let me know if this is the correct way!

@rlweb
Copy link
Contributor Author

rlweb commented Apr 30, 2017 •
edited
Loading

Also, it seems the test pass...!

➜  terraform git:(feature/aws_dynamodb_table-add-timetolive) make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSDynamoDbTable*'      
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/30 23:23:55 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSDynamoDbTable* -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (43.32s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (49.76s)
=== RUN   TestAccAWSDynamoDbTable_basic
	--- PASS: TestAccAWSDynamoDbTable_basic (390.04s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (62.19s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (41.04s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdate
--- PASS: TestAccAWSDynamoDbTable_gsiUpdate (120.53s)
=== RUN   TestAccAWSDynamoDbTable_ttl
--- PASS: TestAccAWSDynamoDbTable_ttl (57.29s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	764.183s

rlweb
rlweb commented Apr 30, 2017
View reviewed changes
builtin/providers/aws/resource_aws_dynamodb_table_test.go Outdated
}

if *ttlDescription.AttributeName != "TestTTL" {
return fmt.Errorf("AttributeName was %s, not TestTTL!", ttlDescription.AttributeName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2 if statements above fail on the build because of the following...
arg ttlDescription.TimeToLiveStatus for printf verb %s of wrong type: *string
Not sure, what's the best way here?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can dereference the pointer in the Errorf argument so the type is truly a string and not *string.

grubernaut
grubernaut approved these changes May 1, 2017
View reviewed changes
Copy link
Contributor

@grubernaut grubernaut left a comment

Choose a reason for hiding this comment

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

LGTM, pending vet fixes

grubernaut
grubernaut approved these changes May 1, 2017
View reviewed changes
Copy link
Contributor

@grubernaut grubernaut 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!

@grubernaut grubernaut merged commit 3a01d5f into hashicorp:master May 1, 2017
@rlweb rlweb deleted the feature/aws_dynamodb_table-add-timetolive branch May 2, 2017 10:20
@radeksimko radeksimko mentioned this pull request May 14, 2017
Closed
glasser
glasser reviewed May 14, 2017
View reviewed changes
website/source/docs/providers/aws/r/dynamodb_table.html.markdown
@@ -72,6 +77,7 @@ The following arguments are supported:
* `type` - One of: S, N, or B for (S)tring, (N)umber or (B)inary data
* `stream_enabled` - (Optional) Indicates whether Streams are to be enabled (true) or disabled (false).
* `stream_view_type` - (Optional) When an item in the table is modified, StreamViewType determines what information is written to the table's stream. Valid values are KEYS_ONLY, NEW_IMAGE, OLD_IMAGE, NEW_AND_OLD_IMAGES.
* `ttl` - (Optional) Indicates whether time to live is enabled (true) or disabled (false) and the `attribute_name` to be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's more typical for the docs to more explicitly list the sub-attributes, eg in a nested list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, opened #14492 to address.

@bglick bglick mentioned this pull request Jul 4, 2017
Closed
sjungwirth
sjungwirth reviewed Sep 22, 2017
View reviewed changes
website/source/docs/providers/aws/r/dynamodb_table.html.markdown
ttl {
attribute_name = "TimeToExist"
enabled = false
}
Copy link

Choose a reason for hiding this comment

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

I was confused by the example attribute name and the way Amazon calls this a Time To Live. In fact the data stored here should be a unix timestamp of when the record expires, not an amount of time (seconds? ms?) for which the data should "live" or "exist". This might be more clear if the attribute_name was something along the lines of "ExpirationTimestamp".

@ghost
Copy link

ghost commented Apr 7, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grubernaut grubernaut grubernaut approved these changes

@sjungwirth sjungwirth sjungwirth left review comments

@glasser glasser glasser left review comments

Assignees
No one assigned
Labels
enhancement provider/aws
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rlweb @glasser @sjungwirth @grubernaut @radeksimko