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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d40501
added fix for issue #167 - defaulting restrictions to none for cloudf…
dpmerron-ltd Feb 14, 2018
ecfea2f
updated cloudfront distribution resource to reflect feedback and ran …
dpmerron-ltd Mar 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions aws/resource_aws_cloudfront_distribution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Set: restrictionsHash,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"geo_restriction": {
Type: schema.TypeSet,
Required: true,
Computed: true,
Set: geoRestrictionHash,
MaxItems: 1,
Elem: &schema.Resource{
Expand All @@ -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

},
},
},
Expand Down
72 changes: 72 additions & 0 deletions aws/resource_aws_cloudfront_distribution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

ri := acctest.RandInt()
testConfig := fmt.Sprintf(testAccAWSCloudFrontDistributionS3ConfigWithNoRestrictions, ri, originBucket, logBucket, testAccAWSCloudFrontDistributionRetainConfig())
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudFrontDistributionDestroy,
Steps: []resource.TestStep{
{
Config: testConfig,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudFrontDistributionExistence(
"aws_cloudfront_distribution.s3_distribution",
),
resource.TestCheckResourceAttr(
"aws_cloudfront_distribution.s3_distribution",
"hosted_zone_id",
"Z2FDTNDATAQYW2",
),
),
},
},
})
}

func TestAccAWSCloudFrontDistribution_S3OriginWithTags(t *testing.T) {
ri := acctest.RandInt()
preConfig := fmt.Sprintf(testAccAWSCloudFrontDistributionS3ConfigWithTags, ri, originBucket, logBucket, testAccAWSCloudFrontDistributionRetainConfig())
Expand Down Expand Up @@ -357,6 +382,53 @@ resource "aws_cloudfront_distribution" "s3_distribution" {
}
`

var testAccAWSCloudFrontDistributionS3ConfigWithNoRestrictions = `
variable rand_id {
default = %d
}

# origin bucket
%s

# log bucket
%s

resource "aws_cloudfront_distribution" "s3_distribution" {
origin {
domain_name = "${aws_s3_bucket.s3_bucket_origin.id}.s3.amazonaws.com"
origin_id = "myS3Origin"
}
enabled = true
default_root_object = "index.html"
logging_config {
include_cookies = false
bucket = "${aws_s3_bucket.s3_bucket_logs.id}.s3.amazonaws.com"
prefix = "myprefix"
}
aliases = [ "mysite.${var.rand_id}.example.com", "yoursite.${var.rand_id}.example.com" ]
default_cache_behavior {
allowed_methods = [ "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT" ]
cached_methods = [ "GET", "HEAD" ]
target_origin_id = "myS3Origin"
forwarded_values {
query_string = false
cookies {
forward = "none"
}
}
viewer_protocol_policy = "allow-all"
min_ttl = 0
default_ttl = 3600
max_ttl = 86400
}
price_class = "PriceClass_200"
viewer_certificate {
cloudfront_default_certificate = true
}
%s
}
`

var testAccAWSCloudFrontDistributionS3ConfigWithTags = `
variable rand_id {
default = %d
Expand Down