jmgreg31 / terraform-aws-cloudfront

AWS Cloudfront Distribution Module
MIT License
27 stars 25 forks source link

Staging #19

Closed jmgreg31 closed 4 years ago

jmgreg31 commented 4 years ago

Testing @schammah changes

jmgreg31 commented 4 years ago

@schammah this build is passing. If you don't have any additional changes in mind for this release, I can merge.

One additional question. If the new proposed syntax is working to iterate through the cache behaviors, why not update for the origins, origin groups, member blocks...etc? Is there a reason you left those, i.e. certain conditions where the more involved iterator is needed?

example of legacy format in origin block:

dynamic "origin" {
    for_each = [for i in var.dynamic_s3_origin_config : {
      name          = i.domain_name
      id            = i.origin_id
      identity      = lookup(i, "origin_access_identity", null)
      path          = lookup(i, "origin_path", null)
      custom_header = lookup(i, "custom_header", null)
    }]
schammah commented 4 years ago

yes i know, few things before we merge

jmgreg31 commented 4 years ago

@schammah sorry, wasn't clear to me what you were asking about the provider.tf but I follow now. I removed it from the module. I added the provider as well create_cf in the example

schammah commented 4 years ago

i think terraform output will result empty unless you apply

schammah commented 4 years ago

before we merge wanna add a few more commits please any chance i get rights to push to staging branch?

jmgreg31 commented 4 years ago

i think terraform output will result empty unless you apply

You're right

before we merge wanna add a few more commits please any chance i get rights to push to staging branch?

The pattern I'd prefer to follow is submitting a PR to the staging branch instead of giving users access to push directly

jmgreg31 commented 4 years ago

@schammah Thanks for the work on this PR and appreciate you working through some of the CI processes.

I added additional_tags and all the outputs to the example for more complete testing in the future. Merging this.