added fix for issue #167 - defaulting restrictions to none for cloudf…

[dpmerron-ltd]

…ront distributions

In reference to: #167

Currently the aws_cloudfront_distribtuion requires

restrictions { geo_restriction { restriction_type = "none" } }

The proposed changes allows this section to be computed.

Acceptance tests output

==> Checking that code complies with gofmt requirements...
go test ./... -timeout=30s -parallel=4
? github.com/terraform-providers/terraform-provider-aws [no test files]
ok github.com/terraform-providers/terraform-provider-aws/aws 3.911s

First PR so go easy on me 👍

[bflad]

Overall this looks like a good change as the CloudFront API documentation states this argument is not required:

Restrictions
A complex type that identifies ways in which you want to restrict distribution of your content.
Type: Restrictions object
Required: No

Can you see my below comments and let me know if you have any questions? Thanks!

[bflad]

Is there a reason you chose Computed: true here instead of Optional: true? Same with below

[bflad]

Nitpick: we should use the SDK provided constant here: Default: cloudfront.GeoRestrictionTypeNone

[bflad]

Since we're changing the attribute from required to optional, I think it would make more sense to actually remove it from all the other test configurations and just ensure we have "_restrictions" test(s) that do actually try to set it.

[bflad]

Also FYI:

Acceptance tests output

==> Checking that code complies with gofmt requirements...
go test ./... -timeout=30s -parallel=4
? github.com/terraform-providers/terraform-provider-aws [no test files]
ok github.com/terraform-providers/terraform-provider-aws/aws 3.911s

This output looks like the Go unit testing framework, which runs really fast across our codebase. The provider acceptance testing suite that actually calls AWS takes a very long time for CloudFront distributions since they take a long time to disable and delete (10-15 minutes each). In this case to run all the CloudFront distribution acceptance tests, I would be running:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudFrontDistribution_'

[dpmerron-ltd]

In reference to all of the above comments @bflad.. I think it is not possible to have optional: true for those options. See the output of the acceptance test below when using Optional: true,

`--- FAIL: TestAccAWSCloudFrontDistribution_importBasic (1239.71s)
testing.go:513: Step 0 error: After applying this step, the plan was not empty:

	DIFF:

	UPDATE: aws_cloudfront_distribution.s3_distribution
	  restrictions.#:                                                      "1" => "0"
	  restrictions.1097372288.geo_restriction.#:                           "1" => "0"
	  restrictions.1097372288.geo_restriction.2625240281.locations.#:      "0" => "0"
	  restrictions.1097372288.geo_restriction.2625240281.restriction_type: "none" => ""`

When using the format I had before (with Computed: True) the tests pass:

=== RUN TestAccAWSCloudFrontDistribution_importBasic --- PASS: TestAccAWSCloudFrontDistribution_importBasic (1240.00s)

Happy to listen to further input you may have.

[bflad]

@danmerron-sig thanks for the followup! That makes sense. Can you please address the other two items in my feedback or let me know if you do not have time for them? 😄 Then this should hopefully be good to go.

[dpmerron-ltd]

I have addressed them I believe but havent had chance yet to let the acceptance test run all the way through. Hoping to do this tomorrow 👍

Just these cloudfront instances take ages to finsih deploying 👎

Dan

[dpmerron-ltd]

Hi @bflad I wonder if you can help here. When running the acceptance tests I get the following;

    restrictions.#:                                                      "1" => "0"
    restrictions.1097372288.geo_restriction.#:                           "1" => "0"
    restrictions.1097372288.geo_restriction.2625240281.locations.#:      "0" => "0"
    restrictions.1097372288.geo_restriction.2625240281.restriction_type: "none" => ""

I believe this is due to a 0 input for restrictions, still actually creates a restriction but sets to state of disabled. Ideally. what I need to do here is set the default to '1' however with a TypeSet this is not possible. Any other ways around this?

Dan

[bflad]

This is complicated since ideally we want to be able to know when the CloudFront distribution restriction configuration diverges from the default "no geo restrictions" one when its not defined in the Terraform configuration. This is not possible when using Computed: true and why we are trying to avoid it here.

I can think of some hacky ways we can accomplish this today, but let me talk with my colleagues to see if we cannot come up with a good way to handle this situation of suppressing the difference in plan.

As an aside, we should leave geo_restrictions as Required: true and MaxItems: 1. Terraform will do the right thing if the parent attribute is Optional by only requiring it when necessary. If I remember correctly, its not valid to declare restrictions without geo_restrictions and its not valid to have multiple geo_restrictions configuration blocks.

[dpmerron-ltd]

Thanks. I am keen to get this sorted and merged to tidy up some of my code... Will be interesting to know what your colleagues have to say about this!

[dpmerron-ltd]

@bflad Was there any update on this?

[zhelding]

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.