provider/aws: CloudFront post-merge review updates

[vancluever]

Hey guys,

Here are the updates to address the code review issues in #5221. Each commit has been pretty well annotated and each commit is kind of an issue in its own, but I can squash if need be.

The updates:

• Fixed the hashing for ACM and IAM certificate parameters.
• Added error handling to complex structure setting
• Fixed primitives to pass pointers in instead of dereferenced values
• Doc updates so that examples are properly indented
• Removed delete retention to ensure that tests play well with Travis

[catsby]

and returns true if the underlying type contains a zero value

It looks like it's returning false for default values

[catsby]

Looks good! One question on the fullOK function and docs, other than that great work 😄

[catsby]

Ah, looks like TestAccAWSCloudFrontDistribution_S3Origin gave me a panic/nil pointer dereference, line 94 of the test file

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x8 pc=0x311a93]

goroutine 20 [running]:
panic(0x1066c20, 0xc82000e140)
    /usr/local/go/src/runtime/panic.go:464 +0x3e6
testing.tRunner.func1(0xc820098a20)
    /usr/local/go/src/testing/testing.go:467 +0x192
panic(0x1066c20, 0xc82000e140)
    /usr/local/go/src/runtime/panic.go:426 +0x4e9
github.com/hashicorp/terraform/builtin/providers/aws.testAccCheckCloudFrontDistributionDestroy(0xc820358f30, 0x0, 0x0)
    /Users/clint/Projects/Go/src/github.com/hashicorp/terraform/builtin/providers/aws/resource_aws_cloudfront_distribution_test.go:94 +0x133

-->     if *dist.DistributionConfig.Enabled != false {
            return fmt.Errorf("CloudFront distribution should be disabled")
        }

[vancluever]

@catsby, checking and fixing now. I'll just remove fullOK, you are right, it's too heavy-handed for this job as this is currently the only function using it. If other things prove to be an issue in hashing, I might look at how to do this better in general. It'd be neat if schema.ResourceData.GetOk() worked for general maps, for example, or more specifically if we had something like that for maps.

[vancluever]

OK, all done. Also added a way to retain the disable-only functionality for tests via setting a TF_TEST_CLOUDFRONT_RETAIN environment variable - let me know your thoughts!

Also, found one more bug while I was at it - the delete waiter was not trapping error messages properly, so I've fixed that now (looks like it's taking abnormally long to disable distributions today).

[catsby]

Thanks! Looks good here and the acc tests are passing, gonna pull this in!

[vancluever]

Thanks @catsby!

[ghost]

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.