provider/aws - CloudFront custom_error_response fixes for missing #6382

jrnt30 · 2016-04-27T19:03:15Z

Omit custom_error_response response_* fields when not explicitly set via config for
SDK call
Adding a test case to ensure that the response_error gets converted
to an empty string properly, versus "0". (Thanks @vancluever)

@vancluever

- Omit custom_error_response response_* fields when not explicitly set via config for SDK call - Adding a test case to ensure that the response_error gets converted to an empty string properly, versus "0". (Thanks @vancluever) Fixes hashicorp#6342

stack72 · 2016-04-27T20:52:37Z

@jrnt30 what issue dies this fix? I don't believe the linked one is correct :)

jrnt30 · 2016-04-27T20:56:55Z

@stack72 Sorry about that, updated.

stack72 · 2016-04-27T21:09:40Z

Nps :) Just want to make sure we link it back to the right bug

stack72 · 2016-04-27T21:14:59Z

builtin/providers/aws/cloudfront_distribution_configuration_structure.go

 		er.ResponseCode = aws.String(strconv.Itoa(v.(int)))
+	} else {
+		er.ResponseCode = aws.String("")


Do we actually want to send an empty string here or just omit it from the request completely?

We do. If the element is missing AWS will error out unless they are present (and in our case empty).

@stack72, as @jrnt30 mentioned, this is actually necessary, because the attribute is TypeInt, but the API and SDK expect string. There's a breakdown in the conversion between @jrnt30 in the referenced issue.

The reasoning for keeping response_code as a TypeInt is three-fold:

It's the HTTP response code that needs to be sent back (and hence can only be int)

It's that way in CloudFormation (which is what I was using as my reference implementation as much as possible)

cloudfront.CustomErrorResponse.ErrorCode (the incoming error code) is *int64, so it's kind of odd that this would be a *string.

As such, we have to set this to "", as the zero value of int, 0, converts to "0" with strconv.Itoa(), versus an empty string.

Got it! Just had to check :)

stack72 · 2016-04-27T21:51:01Z

Currently running the tests on this (they take a long time!) - will post the results when they are done

vancluever · 2016-04-27T22:13:42Z

@stack72 yeah, if you are testing manually, you can set TF_TEST_CLOUDFRONT_RETAIN=1 to get thru the tests faster, if you don't mind deleting the disabled distributions afterwards :)

stack72 · 2016-04-27T22:22:01Z

@jrnt30 / @vancluever

Just a FYI, as the tests were running I just saw this error:

ake testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudFrontDistribution' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudFrontDistribution -timeout 120m
=== RUN   TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (896.46s)
=== RUN   TestAccAWSCloudFrontDistribution_customOrigin
--- PASS: TestAccAWSCloudFrontDistribution_customOrigin (792.49s)
=== RUN   TestAccAWSCloudFrontDistribution_multiOrigin
--- PASS: TestAccAWSCloudFrontDistribution_multiOrigin (1909.30s)
=== RUN   TestAccAWSCloudFrontDistribution_noOptionalItemsConfig
--- FAIL: TestAccAWSCloudFrontDistribution_noOptionalItemsConfig (881.96s)
    testing.go:172: Step 0 error: Check failed: Check 1/1 error: Not found: aws_cloudfront_distribution.no_optional_items
=== RUN   TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig
--- FAIL: TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig (905.15s)
    testing.go:172: Step 0 error: Check failed: Check 1/1 error: Not found: aws_cloudfront_distribution.no_custom_error_responses
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    5385.381s

ok end of run and 2 tests fail - please can you have a look at these

jrnt30 · 2016-04-27T22:45:23Z

Thank you...There will be another, somehow I reversed the names when
merging for the two test case resources.

On Wed, Apr 27, 2016 at 5:22 PM, Paul Stack notifications@github.com
wrote:

@jrnt30 https://github.com/jrnt30 / @vancluever
https://github.com/vancluever

Just a FYI, as the tests were running I just saw this error:

ake testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudFrontDistribution' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudFrontDistribution -timeout 120m
=== RUN TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (896.46s)
=== RUN TestAccAWSCloudFrontDistribution_customOrigin
--- PASS: TestAccAWSCloudFrontDistribution_customOrigin (792.49s)
=== RUN TestAccAWSCloudFrontDistribution_multiOrigin
--- PASS: TestAccAWSCloudFrontDistribution_multiOrigin (1909.30s)
=== RUN TestAccAWSCloudFrontDistribution_noOptionalItemsConfig
--- FAIL: TestAccAWSCloudFrontDistribution_noOptionalItemsConfig (881.96s)
testing.go:172: Step 0 error: Check failed: Check 1/1 error: Not found: aws_cloudfront_distribution.no_optional_items

The last few are still running but at least 1 fails

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6382 (comment)

jrnt30 · 2016-04-27T22:51:07Z

***********
BEFORE: 
***********

17:48 $ TF_TEST_CLOUDFRONT_RETAIN=1 TF_ACC=1 envhcain personal  make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudFrontDistribution' 2>tf.log
==> Checking that code complies with gofmt requirements...
/Users/justinn/Development/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudFrontDistribution -timeout 120m
=== RUN   TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (12.33s)
=== RUN   TestAccAWSCloudFrontDistribution_customOrigin
--- PASS: TestAccAWSCloudFrontDistribution_customOrigin (4.31s)
=== RUN   TestAccAWSCloudFrontDistribution_multiOrigin
--- PASS: TestAccAWSCloudFrontDistribution_multiOrigin (12.97s)
=== RUN   TestAccAWSCloudFrontDistribution_noOptionalItemsConfig
--- FAIL: TestAccAWSCloudFrontDistribution_noOptionalItemsConfig (3.77s)
  testing.go:172: Step 0 error: Check failed: Check 1/1 error: Not found: aws_cloudfront_distribution.no_optional_items
=== RUN   TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig
--- FAIL: TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig (4.00s)
  testing.go:172: Step 0 error: Check failed: Check 1/1 error: Not found: aws_cloudfront_distribution.no_custom_error_responses
FAIL
exit status 1
FAIL  github.com/hashicorp/terraform/builtin/providers/aws  37.408s

***********
AFTER:
***********

17:42 $ TF_TEST_CLOUDFRONT_RETAIN=1 TF_ACC=1 envhcain personal  make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudFrontDistribution' 2>tf.log
==> Checking that code complies with gofmt requirements...
/Users/justinn/Development/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudFrontDistribution -timeout 120m
=== RUN   TestAccAWSCloudFrontDistribution_S3Origin
--- PASS: TestAccAWSCloudFrontDistribution_S3Origin (12.35s)
=== RUN   TestAccAWSCloudFrontDistribution_customOrigin
--- PASS: TestAccAWSCloudFrontDistribution_customOrigin (4.07s)
=== RUN   TestAccAWSCloudFrontDistribution_multiOrigin
--- PASS: TestAccAWSCloudFrontDistribution_multiOrigin (11.38s)
=== RUN   TestAccAWSCloudFrontDistribution_noOptionalItemsConfig
--- PASS: TestAccAWSCloudFrontDistribution_noOptionalItemsConfig (3.92s)
=== RUN   TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig
--- PASS: TestAccAWSCloudFrontDistribution_noCustomErrorResponseConfig (5.05s)
PASS
ok    github.com/hashicorp/terraform/builtin/providers/aws  36.800s

stack72 · 2016-04-27T23:00:15Z

@jrnt30

thanks so much for fixing this up! This LGTM now :)

Paul

@vancluever

…shicorp#6382) * provider/aws - CloudFront custom_error_response fixes for missing - Omit custom_error_response response_* fields when not explicitly set via config for SDK call - Adding a test case to ensure that the response_error gets converted to an empty string properly, versus "0". (Thanks @vancluever) Fixes hashicorp#6342 * - Fixing ACC test case resource names

ghost · 2020-04-26T02:05:02Z

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.

stack72 reviewed Apr 27, 2016
View reviewed changes

stack72 added bug provider/aws labels Apr 27, 2016

stack72 self-assigned this Apr 27, 2016

- Fixing ACC test case resource names

b2e15b1

stack72 merged commit 495c4b3 into hashicorp:master Apr 27, 2016

bobtfish mentioned this pull request May 5, 2016

Rebase from hashicorp/terraform Yelp/terraform#15

Merged

ghost locked and limited conversation to collaborators Apr 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws - CloudFront custom_error_response fixes for missing #6382

provider/aws - CloudFront custom_error_response fixes for missing #6382

jrnt30 commented Apr 27, 2016 •

edited

Loading

stack72 commented Apr 27, 2016

jrnt30 commented Apr 27, 2016

stack72 commented Apr 27, 2016

stack72 Apr 27, 2016

jrnt30 Apr 27, 2016

vancluever Apr 27, 2016

stack72 Apr 27, 2016

stack72 commented Apr 27, 2016

vancluever commented Apr 27, 2016

stack72 commented Apr 27, 2016 •

edited

Loading

jrnt30 commented Apr 27, 2016

jrnt30 commented Apr 27, 2016 •

edited

Loading

stack72 commented Apr 27, 2016

ghost commented Apr 26, 2020

provider/aws - CloudFront custom_error_response fixes for missing #6382

provider/aws - CloudFront custom_error_response fixes for missing #6382

Conversation

jrnt30 commented Apr 27, 2016 • edited Loading

stack72 commented Apr 27, 2016

jrnt30 commented Apr 27, 2016

stack72 commented Apr 27, 2016

stack72 Apr 27, 2016

Choose a reason for hiding this comment

jrnt30 Apr 27, 2016

Choose a reason for hiding this comment

vancluever Apr 27, 2016

Choose a reason for hiding this comment

stack72 Apr 27, 2016

Choose a reason for hiding this comment

stack72 commented Apr 27, 2016

vancluever commented Apr 27, 2016

stack72 commented Apr 27, 2016 • edited Loading

jrnt30 commented Apr 27, 2016

jrnt30 commented Apr 27, 2016 • edited Loading

stack72 commented Apr 27, 2016

ghost commented Apr 26, 2020

jrnt30 commented Apr 27, 2016 •

edited

Loading

stack72 commented Apr 27, 2016 •

edited

Loading

jrnt30 commented Apr 27, 2016 •

edited

Loading