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

CloudFront distribution and origin access identity support #5221

Merged
merged 3 commits into from Apr 14, 2016

Conversation

Projects
None yet
@vancluever
Contributor

vancluever commented Feb 19, 2016

Reference: #3330

I'm opening this new PR just to try and move this along. This branch is pretty much good to merge.

As mentioned in #3330, this is feature-complete CloudFront distribution resource, actually slightly more feature complete than the equivalent in CloudFormation (it doesn't seem like GZIP compression is supported yet, for example). Supports multiple origins, cache behaviours, and has a config layout like you would expect to have when sending the distribution configuration to API or CloudFormation.

I have also added aws_cloudfront_origin_access_identity, which allows origin access identities to be generated from Terraform rather than the console, definitely pushing this feature set past what CloudFormation has.

One other thing I'm waiting on is #5218, which will allow me to enforce a max instance of 1 on some of the complex types that need it (example: default_cache_behavior, viewer_certificate, etc). But if this is merged before that is I will just add these at a later time.

Again, if anyone wants to give it ago in a custom build, if you find anything that's an issue, let me know, otherwise the acceptance tests give a good spread of use cases.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Feb 19, 2016

Contributor

PS: This is failing right now because CloudFront is not yet vendored - if this is necessary, let me know.

Contributor

vancluever commented Feb 19, 2016

PS: This is failing right now because CloudFront is not yet vendored - if this is necessary, let me know.

@whiteadam

This comment has been minimized.

Show comment
Hide comment
@whiteadam

whiteadam commented Feb 22, 2016

+1

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Feb 25, 2016

Contributor

Small update to aws_cloudfront_origin_access_identity:

  • Ignore originAccessIdentityConfig.Comment on update if value is not set (pointer is nil when comment is blank)
Contributor

vancluever commented Feb 25, 2016

Small update to aws_cloudfront_origin_access_identity:

  • Ignore originAccessIdentityConfig.Comment on update if value is not set (pointer is nil when comment is blank)
@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Mar 2, 2016

Contributor

Update! I've added the MaxItems constraint to the appropriate items into the schema now that #5218 has been merged. Should ensure a safer configuration workflow.

Contributor

vancluever commented Mar 2, 2016

Update! I've added the MaxItems constraint to the appropriate items into the schema now that #5218 has been merged. Should ensure a safer configuration workflow.

@shanhchen

This comment has been minimized.

Show comment
Hide comment
@shanhchen

shanhchen Mar 4, 2016

Thanks for this, wondering whether it is possible to add back the zone_id on the output? To make it easier to use CloudFront w/ Route 53 ALIAS records in the DSL:
http://docs.aws.amazon.com/Route53/latest/APIReference/CreateAliasRRSAPI.html
#1775

shanhchen commented Mar 4, 2016

Thanks for this, wondering whether it is possible to add back the zone_id on the output? To make it easier to use CloudFront w/ Route 53 ALIAS records in the DSL:
http://docs.aws.amazon.com/Route53/latest/APIReference/CreateAliasRRSAPI.html
#1775

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Mar 4, 2016

Contributor

@shanhchen sorry, but I don't think that's a good idea.

I was kind of curious as to why it was absent when I was doing my refactor, incidentally, and it turns out that it is a hardcoded value (if you look in the original commit history you can see the static d.Set("zone_id") value. It is not supplied by the CloudFront SDK, nor the API from what I would imagine.

I would say that if you want to reference the CloudFront zone ID, just reference the static value as instructed in the docs.

Contributor

vancluever commented Mar 4, 2016

@shanhchen sorry, but I don't think that's a good idea.

I was kind of curious as to why it was absent when I was doing my refactor, incidentally, and it turns out that it is a hardcoded value (if you look in the original commit history you can see the static d.Set("zone_id") value. It is not supplied by the CloudFront SDK, nor the API from what I would imagine.

I would say that if you want to reference the CloudFront zone ID, just reference the static value as instructed in the docs.

@shanhchen

This comment has been minimized.

Show comment
Hide comment
@shanhchen

shanhchen Mar 4, 2016

Thanks a lot @vancluever I will try that way.

shanhchen commented Mar 4, 2016

Thanks a lot @vancluever I will try that way.

@catsby

This comment has been minimized.

Show comment
Hide comment
@catsby

catsby Mar 8, 2016

Member

Apologies for the silence here. Should I be looking here, or #3330 ? I think this is a replacement for that one, right?

Member

catsby commented Mar 8, 2016

Apologies for the silence here. Should I be looking here, or #3330 ? I think this is a replacement for that one, right?

@catsby

This comment has been minimized.

Show comment
Hide comment
@catsby

catsby Mar 8, 2016

Member

Also, as mentioned above, this needs the CloudFront dependency vendored.. See here for details

Thanks!

Member

catsby commented Mar 8, 2016

Also, as mentioned above, this needs the CloudFront dependency vendored.. See here for details

Thanks!

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Mar 8, 2016

Contributor

Hey @catsby, great to hear this is getting attention now. This is indeed the replacement, so this is the one that should be reviewed. I encountered a bug that needs fixing the last couple of days, I'll add that, in addition to the vendoring of the CloudFront SDK bits today.

Contributor

vancluever commented Mar 8, 2016

Hey @catsby, great to hear this is getting attention now. This is indeed the replacement, so this is the one that should be reviewed. I encountered a bug that needs fixing the last couple of days, I'll add that, in addition to the vendoring of the CloudFront SDK bits today.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Mar 9, 2016

Contributor

OK, bug fixes are in and CloudFront deps vendored. Not too sure what happened to the travis job, was hoping to see if it passed tests. But this should be good to go regardless.

Contributor

vancluever commented Mar 9, 2016

OK, bug fixes are in and CloudFront deps vendored. Not too sure what happened to the travis job, was hoping to see if it passed tests. But this should be good to go regardless.

@AlexanderEkdahl

This comment has been minimized.

Show comment
Hide comment
@AlexanderEkdahl

AlexanderEkdahl Mar 10, 2016

Contributor

Impressive work!

Contributor

AlexanderEkdahl commented Mar 10, 2016

Impressive work!

@joshuaspence

This comment has been minimized.

Show comment
Hide comment
@joshuaspence

joshuaspence Mar 15, 2016

Contributor

Looking forward to it

Contributor

joshuaspence commented Mar 15, 2016

Looking forward to it

@nickschuch

This comment has been minimized.

Show comment
Hide comment
@nickschuch

nickschuch Mar 22, 2016

Anything I can do to help? Super keen to get this over the line!

nickschuch commented Mar 22, 2016

Anything I can do to help? Super keen to get this over the line!

tmatilai added a commit to Yleisradio/homebrew-terraforms that referenced this pull request Mar 22, 2016

Add Terraform 0.6.14+cf
Custom build of the CloudFront support pull request (hashicorp/terraform#5221)
rebased onto v0.6.14 release.
@tmatilai

This comment has been minimized.

Show comment
Hide comment
@tmatilai

tmatilai Mar 22, 2016

I rebased this PR on top of the recent 0.6.14 release and put the OS X package available here if someone wants to test it.

There is also a fancy Terraform version switcher: Yleisradio/homebrew-terraforms
With it you can just say chtf 0.6.14+cf.

tmatilai commented Mar 22, 2016

I rebased this PR on top of the recent 0.6.14 release and put the OS X package available here if someone wants to test it.

There is also a fancy Terraform version switcher: Yleisradio/homebrew-terraforms
With it you can just say chtf 0.6.14+cf.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Mar 22, 2016

Contributor

@tmatilai - Thanks for the review, documentation for aws_cloudfront_origin_access_identity has now been updated, squashed and pushed. Neat tool too btw!

And thanks everyone for the great feedback and support!

Contributor

vancluever commented Mar 22, 2016

@tmatilai - Thanks for the review, documentation for aws_cloudfront_origin_access_identity has now been updated, squashed and pushed. Neat tool too btw!

And thanks everyone for the great feedback and support!

@tmatilai

This comment has been minimized.

Show comment
Hide comment
@tmatilai

tmatilai Mar 30, 2016

@vancluever any chance adding support also for ACM certificates? =)

tmatilai commented Mar 30, 2016

@vancluever any chance adding support also for ACM certificates? =)

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Mar 30, 2016

Contributor

@tmatilai, I just added support for ACM certificates - can you check though and tell me if it works for you? It's hard to do acceptance tests for these things without a valid ACM or IAM cert, of course, so I can't automate this. Let me know if it looks good and I'll squash, or send along the issue you're running into.

Contributor

vancluever commented Mar 30, 2016

@tmatilai, I just added support for ACM certificates - can you check though and tell me if it works for you? It's hard to do acceptance tests for these things without a valid ACM or IAM cert, of course, so I can't automate this. Let me know if it looks good and I'll squash, or send along the issue you're running into.

@tmatilai

This comment has been minimized.

Show comment
Hide comment
@tmatilai

tmatilai Mar 30, 2016

I just added support for ACM certificates

Awesome, really appreciated! I'll build and test tomorrow and let you know.

tmatilai commented Mar 30, 2016

I just added support for ACM certificates

Awesome, really appreciated! I'll build and test tomorrow and let you know.

@blakesmith

This comment has been minimized.

Show comment
Hide comment
@blakesmith

blakesmith Apr 1, 2016

Contributor

Oh, very cool. I was only going through the comments. Thank you!

Contributor

blakesmith commented Apr 1, 2016

Oh, very cool. I was only going through the comments. Thank you!

@tmatilai

This comment has been minimized.

Show comment
Hide comment
@tmatilai

tmatilai Apr 1, 2016

@vancluever to be able to merge (via Github), you should also rebase on top of the current master, as there are conflicts in at least resource definition.

tmatilai commented Apr 1, 2016

@vancluever to be able to merge (via Github), you should also rebase on top of the current master, as there are conflicts in at least resource definition.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 1, 2016

Contributor

Yeah there were definitely a few.

I've done the rebase now and tested, everything passes, and it should be easier to play with for the time being again. UPDATE: Just fixed the CloudFront rev in the Godeps as well.

If I make any more changes before it gets merged I'll rebase off of master again just to keep this stuff up to date.

Contributor

vancluever commented Apr 1, 2016

Yeah there were definitely a few.

I've done the rebase now and tested, everything passes, and it should be easier to play with for the time being again. UPDATE: Just fixed the CloudFront rev in the Godeps as well.

If I make any more changes before it gets merged I'll rebase off of master again just to keep this stuff up to date.

@blakesmith

This comment has been minimized.

Show comment
Hide comment
@blakesmith

blakesmith Apr 2, 2016

Contributor

@vancluever Was able to use your latest branch to setup a S3 static site fronted with CloudFront. Everything worked perfectly. Thanks for the patches!

Contributor

blakesmith commented Apr 2, 2016

@vancluever Was able to use your latest branch to setup a S3 static site fronted with CloudFront. Everything worked perfectly. Thanks for the patches!

@blakesmith

This comment has been minimized.

Show comment
Hide comment
@blakesmith

blakesmith Apr 4, 2016

Contributor

If anyone else is interested in trying out this PR, I wrote up a blog post on using this to setup an S3 static site fronted with CloudFront: http://blakesmith.me/2016/04/02/terraform-aws-static-site-with-cloudfront.html

Contributor

blakesmith commented Apr 4, 2016

If anyone else is interested in trying out this PR, I wrote up a blog post on using this to setup an S3 static site fronted with CloudFront: http://blakesmith.me/2016/04/02/terraform-aws-static-site-with-cloudfront.html

@gpetras

This comment has been minimized.

Show comment
Hide comment
@gpetras

gpetras Apr 6, 2016

I've tested this out and so far it works great. One question I have - how do I setup the default cache behavior to forward all headers? In the UI there's a dropdown to select 'All' but I can't figure out how to get the same effect in my default_cache_behavior. The headers parameter accepts a list. It'd be great if it accepted a keyword, ie headers = "all", or something to that effect. Thanks for all your work @vancluever!

gpetras commented Apr 6, 2016

I've tested this out and so far it works great. One question I have - how do I setup the default cache behavior to forward all headers? In the UI there's a dropdown to select 'All' but I can't figure out how to get the same effect in my default_cache_behavior. The headers parameter accepts a list. It'd be great if it accepted a keyword, ie headers = "all", or something to that effect. Thanks for all your work @vancluever!

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 6, 2016

Contributor

Hey @gpetras you are welcome! Also, I got this from the SDK docs:

If you want CloudFront to forward all headers to the origin and vary on all of them, specify 1 for Quantity and * for Name.

So you want to specify * for all headers.

PS: I've updated the docs to be specific on this.

Contributor

vancluever commented Apr 6, 2016

Hey @gpetras you are welcome! Also, I got this from the SDK docs:

If you want CloudFront to forward all headers to the origin and vary on all of them, specify 1 for Quantity and * for Name.

So you want to specify * for all headers.

PS: I've updated the docs to be specific on this.

AlexanderEkdahl and others added some commits Sep 26, 2015

Chris Marchesi
Refactor - new resource: aws_cloudfront_distribution
 * Includes a complete re-write of the old aws_cloudfront_web_distribution
   resource to bring it to feature parity with API and CloudFormation.
 * Also includes the aws_cloudfront_origin_access_identity resource to generate
   origin access identities for use with S3.
@gpetras

This comment has been minimized.

Show comment
Hide comment
@gpetras

gpetras Apr 7, 2016

Thanks @vancluever - I did that and it worked great - much appreciated!

gpetras commented Apr 7, 2016

Thanks @vancluever - I did that and it worked great - much appreciated!

@liviudm

This comment has been minimized.

Show comment
Hide comment
@liviudm

liviudm Apr 8, 2016

Hi,

When I'm trying to specify an ACM ARN I get the following error:

Errors:

  * aws_cloudfront_distribution.liviu_io_distribution: cannot parse '' as bool: strconv.ParseBool: parsing "arn:aws:acm:us-east-1:XXXXXX:certificate/ee12ab44-25bd-4bd4-b00f-d9f0a3a8ad68": invalid syntax

Here's the relevant part from my configuration:

viewer_certificate {
        acm_certificate_arn = "arn:aws:acm:us-east-1:XXXXXX:certificate/ee12ab44-25bd-4bd4-b00f-d9f0a3a8ad68"
        minimum_protocol_version = "TLSv1"
        ssl_support_method = "sni-only"
}

However, there are no errors if I set acm_certificate_arn to true. Here's the relevant output from terraform plan:

    viewer_certificate.#:                                                                                 "0" => "1"
    viewer_certificate.103543815.acm_certificate_arn:                                                     "" => "1"
    viewer_certificate.103543815.cloudfront_default_certificate:                                          "" => ""
    viewer_certificate.103543815.iam_certificate_id:                                                      "" => ""
    viewer_certificate.103543815.minimum_protocol_version:                                                "" => "TLSv1"
    viewer_certificate.103543815.ssl_support_method:                                                      "" => "sni-only"

Also, when trying to apply the configuration:

Error applying plan:

1 error(s) occurred:

* aws_cloudfront_distribution.liviu_io_distribution: unexpected EOF

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

I guess this isn't the intended behavior and I should specify a valid ACM ARN as the value for acm_certificate_arn, not true or false.

liviudm commented Apr 8, 2016

Hi,

When I'm trying to specify an ACM ARN I get the following error:

Errors:

  * aws_cloudfront_distribution.liviu_io_distribution: cannot parse '' as bool: strconv.ParseBool: parsing "arn:aws:acm:us-east-1:XXXXXX:certificate/ee12ab44-25bd-4bd4-b00f-d9f0a3a8ad68": invalid syntax

Here's the relevant part from my configuration:

viewer_certificate {
        acm_certificate_arn = "arn:aws:acm:us-east-1:XXXXXX:certificate/ee12ab44-25bd-4bd4-b00f-d9f0a3a8ad68"
        minimum_protocol_version = "TLSv1"
        ssl_support_method = "sni-only"
}

However, there are no errors if I set acm_certificate_arn to true. Here's the relevant output from terraform plan:

    viewer_certificate.#:                                                                                 "0" => "1"
    viewer_certificate.103543815.acm_certificate_arn:                                                     "" => "1"
    viewer_certificate.103543815.cloudfront_default_certificate:                                          "" => ""
    viewer_certificate.103543815.iam_certificate_id:                                                      "" => ""
    viewer_certificate.103543815.minimum_protocol_version:                                                "" => "TLSv1"
    viewer_certificate.103543815.ssl_support_method:                                                      "" => "sni-only"

Also, when trying to apply the configuration:

Error applying plan:

1 error(s) occurred:

* aws_cloudfront_distribution.liviu_io_distribution: unexpected EOF

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

I guess this isn't the intended behavior and I should specify a valid ACM ARN as the value for acm_certificate_arn, not true or false.

@tmatilai

This comment has been minimized.

Show comment
Hide comment
@tmatilai

tmatilai Apr 8, 2016

@liviudm I think you have an old version of the pull request. That should be fixed.
Also note that the fixes are squashed (and rebased), so you're gonna need to hard reset your working tree after fetching.

tmatilai commented Apr 8, 2016

@liviudm I think you have an old version of the pull request. That should be fixed.
Also note that the fixes are squashed (and rebased), so you're gonna need to hard reset your working tree after fetching.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 8, 2016

Contributor

@liviudm as @tmatilai mentioned, this was caused by a bug in the first push for the ACM feature (if you see the outdated commit comments you can see Teemu's comments in there). Updating your tree as per his instructions should work and I verified the present commit to make sure there wasn't a regression.

Contributor

vancluever commented Apr 8, 2016

@liviudm as @tmatilai mentioned, this was caused by a bug in the first push for the ACM feature (if you see the outdated commit comments you can see Teemu's comments in there). Updating your tree as per his instructions should work and I verified the present commit to make sure there wasn't a regression.

prefix = "myprefix"
}
aliases = [ "mysite.example.com", "yoursite.example.com" ]
default_cache_behavior {

This comment has been minimized.

@bradleyayers

bradleyayers Apr 10, 2016

In the documentation below it says that path_pattern is required, yet in this example it is absent. One or the other should be updated. Edit: Oops nevermind, I missed the default_cache_behavior section that clearly states path_pattern isn't required.

@bradleyayers

bradleyayers Apr 10, 2016

In the documentation below it says that path_pattern is required, yet in this example it is absent. One or the other should be updated. Edit: Oops nevermind, I missed the default_cache_behavior section that clearly states path_pattern isn't required.

domain_name = "mybucket.s3.amazonaws.com"
origin_id = "myS3Origin"
s3_origin_config {
origin_access_identity = "origin-access-identity/cloudfront/ABCDEFG1234567"

This comment has been minimized.

@bradleyayers

bradleyayers Apr 10, 2016

The indentation on these two lines is wrong.

@bradleyayers

bradleyayers Apr 10, 2016

The indentation on these two lines is wrong.

@bradleyayers

This comment has been minimized.

Show comment
Hide comment
@bradleyayers

bradleyayers Apr 11, 2016

@vancluever I'm having an issue with migrating from

  viewer_certificate {
    cloudfront_default_certificate = true
  }

to

  viewer_certificate {
    acm_certificate_arn = "…"
  }

When I run terraform plan there are no changes.

bradleyayers commented Apr 11, 2016

@vancluever I'm having an issue with migrating from

  viewer_certificate {
    cloudfront_default_certificate = true
  }

to

  viewer_certificate {
    acm_certificate_arn = "…"
  }

When I run terraform plan there are no changes.

//
// Used by the aws_cloudfront_distribution Read function.
func flattenDistributionConfig(d *schema.ResourceData, distributionConfig *cloudfront.DistributionConfig) {
d.Set("origins", flattenOrigins(distributionConfig.Origins))

This comment has been minimized.

@catsby

catsby Apr 14, 2016

Member

for anything that is not a simple value like int or string, we need to check the error from d.Set to ensure it was set correctly. This applies to all the d.Set's here where a complex structure is being saved

@catsby

catsby Apr 14, 2016

Member

for anything that is not a simple value like int or string, we need to check the error from d.Set to ensure it was set correctly. This applies to all the d.Set's here where a complex structure is being saved

// Used by the aws_cloudfront_distribution Read function.
func flattenDistributionConfig(d *schema.ResourceData, distributionConfig *cloudfront.DistributionConfig) {
d.Set("origins", flattenOrigins(distributionConfig.Origins))
d.Set("enabled", *distributionConfig.Enabled)

This comment has been minimized.

@catsby

catsby Apr 14, 2016

Member

This isn't a blocker, but d.Set can safely dereference pointers, so it's considered better to just send the pointer here:

d.Set("enabled", distributionConfig.Enabled)
@catsby

catsby Apr 14, 2016

Member

This isn't a blocker, but d.Set can safely dereference pointers, so it's considered better to just send the pointer here:

d.Set("enabled", distributionConfig.Enabled)
}
// skip delete if retain_on_delete is enabled
if d.Get("retain_on_delete").(bool) {

This comment has been minimized.

@catsby

catsby Apr 14, 2016

Member

This is kind of an odd parameter. Is the intent here to allow people to delete the resource, but not actually wait the entire time it takes to fully destroy it? If so, then we need to call delete here and then just simply not wait.

As it stands, we're simply removing it from state. Is that the intended behavior?

I'd rather see this renamed to something that indicates we don't wait for confirmation, instead we call delete and verify no error is returned, then remove it from state and carry on.

@catsby

catsby Apr 14, 2016

Member

This is kind of an odd parameter. Is the intent here to allow people to delete the resource, but not actually wait the entire time it takes to fully destroy it? If so, then we need to call delete here and then just simply not wait.

As it stands, we're simply removing it from state. Is that the intended behavior?

I'd rather see this renamed to something that indicates we don't wait for confirmation, instead we call delete and verify no error is returned, then remove it from state and carry on.

This comment has been minimized.

@catsby

catsby Apr 14, 2016

Member

Ok, reading more on retain_on_delete, I see that we have to disable it in order to destroy it, and the act of disabling it can take 15 minutes, is that correct? Ok. I read the docs on AWS site, seems like a cumbersome ordeal

@catsby

catsby Apr 14, 2016

Member

Ok, reading more on retain_on_delete, I see that we have to disable it in order to destroy it, and the act of disabling it can take 15 minutes, is that correct? Ok. I read the docs on AWS site, seems like a cumbersome ordeal

This comment has been minimized.

@vancluever

vancluever Apr 14, 2016

Contributor

Hey @catsby, yeah, that's the intention here. Doing acceptance tests on this resource would have been extremely painful without this, and also in some circumstances you might want to set this on code you are working on, and maybe even production if you don't have any other resources dependent on it (it does cause conflict with the aws_cloudfront_origin_access_identity resource, for example, as an access identity can't be deleted if it's associated with a distribution).

@vancluever

vancluever Apr 14, 2016

Contributor

Hey @catsby, yeah, that's the intention here. Doing acceptance tests on this resource would have been extremely painful without this, and also in some circumstances you might want to set this on code you are working on, and maybe even production if you don't have any other resources dependent on it (it does cause conflict with the aws_cloudfront_origin_access_identity resource, for example, as an access identity can't be deleted if it's associated with a distribution).

This comment has been minimized.

@catsby

catsby Apr 15, 2016

Member

Doing acceptance tests on this resource would have been extremely painful without this

hrm, we run these nightly in Travis. Does that mean we have a leak? Waiting 15+ minutes for deletion is not a problem for machines

@catsby

catsby Apr 15, 2016

Member

Doing acceptance tests on this resource would have been extremely painful without this

hrm, we run these nightly in Travis. Does that mean we have a leak? Waiting 15+ minutes for deletion is not a problem for machines

This comment has been minimized.

@vancluever

vancluever Apr 15, 2016

Contributor

Hey @catsby, yeah it might be something we want to remove from the acceptance tests then, lest the test account get cluttered with disabled distributions. I can add this to the work I'm doing on the rest of the code review stuff if you want.

EDIT: Maybe we can figure out a way to add this only to interactive tests?

@vancluever

vancluever Apr 15, 2016

Contributor

Hey @catsby, yeah it might be something we want to remove from the acceptance tests then, lest the test account get cluttered with disabled distributions. I can add this to the work I'm doing on the rest of the code review stuff if you want.

EDIT: Maybe we can figure out a way to add this only to interactive tests?

@catsby

This comment has been minimized.

Show comment
Hide comment
@catsby

catsby Apr 14, 2016

Member

Going to go ahead and merge this as is. My feedback was pretty minimal and can be picked up as we go.

Thanks so much for the work here @vancluever and @AlexanderEkdahl !

Member

catsby commented Apr 14, 2016

Going to go ahead and merge this as is. My feedback was pretty minimal and can be picked up as we go.

Thanks so much for the work here @vancluever and @AlexanderEkdahl !

@catsby catsby merged commit a38ccbe into hashicorp:master Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@feanil

This comment has been minimized.

Show comment
Hide comment
@feanil

feanil Apr 14, 2016

Contributor

@catsby how long before this is available in a new version of terraform?

Contributor

feanil commented Apr 14, 2016

@catsby how long before this is available in a new version of terraform?

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 14, 2016

Contributor

Amazing! Thanks @catsby!

@bradleyayers - will look into your and @catsby's feedback soon, including attempting to reproduce the diff issue (gotta set up a few things so I can do ACM easily enough).

Contributor

vancluever commented Apr 14, 2016

Amazing! Thanks @catsby!

@bradleyayers - will look into your and @catsby's feedback soon, including attempting to reproduce the diff issue (gotta set up a few things so I can do ACM easily enough).

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 15, 2016

Contributor

@bradleyayers just a note - I'm working on the issue you reported and it looks like it mainly comes up if you only specify acm_certificate_arn and no other parameters - it doesn't look like the hash is being calculated properly on that set item. Not too sure why it's not hashing, but in the meantime, if you want to move forward, just add ssl_support_method, which needs to be there anyway for ACM and IAM certificates.

Contributor

vancluever commented Apr 15, 2016

@bradleyayers just a note - I'm working on the issue you reported and it looks like it mainly comes up if you only specify acm_certificate_arn and no other parameters - it doesn't look like the hash is being calculated properly on that set item. Not too sure why it's not hashing, but in the meantime, if you want to move forward, just add ssl_support_method, which needs to be there anyway for ACM and IAM certificates.

chrislovecnm added a commit to chrislovecnm/terraform that referenced this pull request Apr 16, 2016

CloudFront distribution and origin access identity support (#5221)
* CloudFront implementation v3

* Update tests

* Refactor - new resource: aws_cloudfront_distribution

 * Includes a complete re-write of the old aws_cloudfront_web_distribution
   resource to bring it to feature parity with API and CloudFormation.
 * Also includes the aws_cloudfront_origin_access_identity resource to generate
   origin access identities for use with S3.

@vancluever vancluever deleted the paybyphone:cloudfront_v3 branch Apr 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment