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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dpmerron-ltd
Copy link

…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 👍

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Feb 14, 2018
@radeksimko radeksimko added bug Addresses a defect in current functionality. service/cloudfront Issues and PRs that pertain to the cloudfront service. labels Feb 14, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

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!

@@ -432,14 +432,14 @@ func resourceAwsCloudFrontDistribution() *schema.Resource {
},
"restrictions": {
Type: schema.TypeSet,
Required: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -451,7 +451,8 @@ func resourceAwsCloudFrontDistribution() *schema.Resource {
},
"restriction_type": {
Type: schema.TypeString,
Required: true,
Optional: true,
Default: "none",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -44,6 +44,31 @@ func TestAccAWSCloudFrontDistribution_S3Origin(t *testing.T) {
})
}

func TestAccAWSCloudFrontDistribution_S3OriginNoRestrictions(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

bflad commented Feb 15, 2018

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_'

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 15, 2018
@dpmerron-ltd
Copy link
Author

dpmerron-ltd commented Feb 17, 2018

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 bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 19, 2018
@bflad
Copy link
Contributor

bflad commented Feb 19, 2018

@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
Copy link
Author

dpmerron-ltd commented Feb 19, 2018

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

@ghost ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Mar 27, 2018
@dpmerron-ltd
Copy link
Author

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 bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed bug Addresses a defect in current functionality. labels Mar 28, 2018
@bflad
Copy link
Contributor

bflad commented Mar 28, 2018

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.

@bflad bflad added the thinking label Mar 28, 2018
@dpmerron-ltd
Copy link
Author

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
Copy link
Author

@bflad Was there any update on this?

@aeschright aeschright requested a review from a team June 25, 2019 19:25
Base automatically changed from master to main January 23, 2021 00:55
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:55
@zhelding
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/cloudfront Issues and PRs that pertain to the cloudfront service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants