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: Ensure ttl is properly read #2452

Merged
merged 2 commits into from
Dec 12, 2017

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Nov 28, 2017

Closes #2442

Previously it was pointing to a non-existent timeToLive attribute and not returning any errors.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSDynamoDbTable'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDynamoDbTable -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (63.75s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (61.60s)
=== RUN   TestAccAWSDynamoDbTable_importTimeToLive
--- PASS: TestAccAWSDynamoDbTable_importTimeToLive (56.04s)
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (324.05s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (51.13s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (56.07s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdate
--- PASS: TestAccAWSDynamoDbTable_gsiUpdate (195.70s)
=== RUN   TestAccAWSDynamoDbTable_ttl
--- PASS: TestAccAWSDynamoDbTable_ttl (71.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	879.650s

@bflad bflad mentioned this pull request Nov 28, 2017
@radeksimko radeksimko added the bug Addresses a defect in current functionality. label Nov 30, 2017
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Just 1 question + one suggestion.

"attribute_name": *timeToLiveOutput.TimeToLiveDescription.AttributeName,
"enabled": (*timeToLiveOutput.TimeToLiveDescription.TimeToLiveStatus == dynamodb.TimeToLiveStatusEnabled),
}
timeToLiveList = append(timeToLiveList, timeToLiveSet)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: As we know we're only ever going to need 1 element of the slice we can make it a fixed-size array:

timeToLiveList := [...]interface{}{
	map[string]interface{}{
		"attribute_name": *timeToLiveOutput.TimeToLiveDescription.AttributeName,
		"enabled":        (*timeToLiveOutput.TimeToLiveDescription.TimeToLiveStatus == dynamodb.TimeToLiveStatusEnabled),
	},
}


log.Printf("[DEBUG] Loaded TimeToLive data for DynamoDB table '%s'", d.Id())
if timeToLiveOutput.TimeToLiveDescription.AttributeName != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we're checking timeToLiveOutput.TimeToLiveDescription.AttributeName instead of timeToLiveOutput.TimeToLiveDescription here?

I guess we can assume AttributeName is not nil if the parent struct is not nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot actually. 😄 It causes a panic on AttributeName dereference in the basic acceptance testing without the additional nil check. I will update it to check both the parent struct and AttributeName

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2537d6f]

goroutine 375 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.flattenAwsDynamoDbTableResource(0xc42019b650, 0x3217440, 0xc420532480, 0xc4207e8160, 0x0, 0x5168700)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_dynamodb_table.go:823 +0x1a1f

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 11, 2017
* Add timeToLiveOutput.TimeToLiveDescription nil check
* Simplify logic to d.Set ttl
@bflad
Copy link
Contributor Author

bflad commented Dec 11, 2017

@radeksimko updated!

  • Add timeToLiveOutput.TimeToLiveDescription nil check
  • Simplify logic to d.Set ttl
make testacc TEST=./aws TESTARGS='-run=TestAccAWSDynamoDbTable'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDynamoDbTable -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (58.48s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (73.62s)
=== RUN   TestAccAWSDynamoDbTable_importTimeToLive
--- PASS: TestAccAWSDynamoDbTable_importTimeToLive (63.36s)
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (387.61s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (81.41s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (59.59s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdate
--- PASS: TestAccAWSDynamoDbTable_gsiUpdate (188.32s)
=== RUN   TestAccAWSDynamoDbTable_ttl
--- PASS: TestAccAWSDynamoDbTable_ttl (53.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	965.599s


log.Printf("[DEBUG] Loaded TimeToLive data for DynamoDB table '%s'", d.Id())
if timeToLiveOutput.TimeToLiveDescription != nil && timeToLiveOutput.TimeToLiveDescription.AttributeName != nil {
timeToLiveList := []interface{}{
Copy link
Member

Choose a reason for hiding this comment

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

This is certainly more compact, but it's still a slice, not an array. The three dots I mentioned in my example were turning it into an array.

https://blog.golang.org/go-slices-usage-and-internals

It's certainly not a big deal though.

@radeksimko radeksimko merged commit edb26df into hashicorp:master Dec 12, 2017
psyvision added a commit to psyvision/terraform-provider-aws that referenced this pull request Dec 12, 2017
* Add Data Source: aws_elb

* Fix dataSourceAwsElb typo

* Fix dataSourceAwsElb Schema name field to not include Computed

* Remove dataSourceAwsElb schema defaults for computed fields

* Remove dataSourceAwsElb schema defaults for nested computed fields too

* Corrected depends_on entry for EIP

  depends_on                = ["aws_internet_gateway.gw"] is the correct syntax

* Add sweeper for IAM Server Certificates

* test/aws_config_delivery_channel: Add missing dependencies

* d/aws_elb r/aws_elb: hashicorp#2004 review comments

* Remove enable_deletion_protection from testAccDataSourceAWSELBConfigBasic
* Replace unnecessary errwrap.Wrapf with fmt.Errorf
* Reduce flattenAwsELbResource to ec2conn and elbconn instead of meta
* Properly name TestAccDataSourceAWSELB_basic resources
* Use t.Name() for description and TestName tags

* d/aws_elb: Fix documentation sidebar ordering after merging master with new d/aws_elasticache_replication_group

* Makefile: Add sweep target

* Update cognito_user_pool.markdown

* Update CHANGELOG.md

* r/aws_elasticache_security_group: add import support (hashicorp#2277)

* r/aws_elasticache_security_group: add import support

* r/aws_elasticache_security_group: hashicorp#2277 review updates

* Use d.Id() instead of d.Get("name") on read, which allows using schema.ImportStatePassthrough
* d.Set("security_group_names") on read
* Set AWS_DEFAULT_REGION to us-east-1 on import testing

* Update CHANGELOG.md

* documentation: remove antislashes in page titles

* Added missing WARN debug lines when reading a non-existing resource

* Removed <wbr> from documentation titles

* vendor: Bump aws-sdk-go to v.1.12.44

* Add logs for iam server certificate delete conflict (hashicorp#2533)

* Query elb API for load balancer arn causing delete conflict

- For IAM server certificate.

* Use regex for lb name.

* Edits for hashicorp#2533

* r/aws_sqs_queue_policy: Support import by queue URL (hashicorp#2544)

* Update CHANGELOG.md

* r/aws_elasticsearch_domain: Add LogPublishingOption (hashicorp#2285)

* WIP

* Add enabled

* Use cwl policy

* Reflect reviews

* Update CHANGELOG.md

* Add force_destroy field to aws_athena_database (hashicorp#2363)

* Add force_destroy field to aws_athena_database.

Fixes hashicorp#2362.

* Remove unnecessary import.

* Code review feedback

* Update CHANGELOG.md

* Add more example and missing field

* New Resource: aws_media_store_container (hashicorp#2448)

* New Resource: aws_media_store_container

* Reflect reviews

* remove policy

* Update CHANGELOG.md

* Add Redis AUTH, in-transit and at-rest encryption (hashicorp#2090)

* add AUTH, at-rest and in-transit encryption to Elasticache replication groups

* add _enabled to transit/at_rest encyrption parameters

* added one more _enabled

* move validateAwsElastiCacheReplicationGroupAuthToken to aws/validators.go, as well as tests

* set auth_token to nil during Reads

* update Replication Group encryption acceptance tests to use config functions instead of vars

* Fix whitespacing (tabs -> spaces)

* docs/elasticache_replication_group: Add missing fields

* Update CHANGELOG.md

* r/aws_dynamodb_table: Ensure ttl is properly read (hashicorp#2452)

* r/aws_dynamodb_table: Ensure ttl is properly read

* r/aws_dynamodb_table: hashicorp#2452 review updates

* Add timeToLiveOutput.TimeToLiveDescription nil check
* Simplify logic to d.Set ttl

* Update CHANGELOG.md

* Bump aws-sdk-go to v.1.12.45

* New Resource: PublicDnsNamespace (hashicorp#2569)

* WIP

* Add test, docs

* Reflect reviews

* Modify error handling

* Update CHANGELOG.md

* New Resource: ServiceDiscovery PrivateDNS Namespace (hashicorp#2589)

* New Resource: service_discovery_private_dns_namespace

* Reflect reviews

* Update CHANGELOG.md
psyvision added a commit to psyvision/terraform-provider-aws that referenced this pull request Dec 13, 2017
* Makefile: Add sweep target

* Add more example and missing field

* docs/elasticache_replication_group: Add missing fields

* Update CHANGELOG.md

* r/aws_dynamodb_table: Ensure ttl is properly read (hashicorp#2452)

* r/aws_dynamodb_table: Ensure ttl is properly read

* r/aws_dynamodb_table: hashicorp#2452 review updates

* Add timeToLiveOutput.TimeToLiveDescription nil check
* Simplify logic to d.Set ttl

* Update CHANGELOG.md

* Bump aws-sdk-go to v.1.12.45

* New Resource: PublicDnsNamespace (hashicorp#2569)

* WIP

* Add test, docs

* Reflect reviews

* Modify error handling

* Update CHANGELOG.md

* New Resource: ServiceDiscovery PrivateDNS Namespace (hashicorp#2589)

* New Resource: service_discovery_private_dns_namespace

* Reflect reviews

* Update CHANGELOG.md

* hashicorp#2217: re-enable default encryption after disabling in via UI

* hashicorp#2217: documentation
@ghost
Copy link

ghost commented Apr 10, 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 10, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DynamoDB ttl refresh
3 participants