#pr-reviews (2022-01)

Pull Request Reviews for Cloud Posse Projects

2022-01-11

Frank avatar
fix: set viewer_certificate.ssl_support_method to a non-empty value by syphernl · Pull Request #208 · cloudposse/terraform-aws-cloudfront-s3-cdnattachment image

what Sets the viewer_certificate.ssl_support_method to a non-empty value why Since AWS Provider 3.71.0 the viewer_certificate.ssl_support_method is being validated on plan-time and can no longer…

feat: add origin-shield by syphernl · Pull Request #207 · cloudposse/terraform-aws-cloudfront-s3-cdnattachment image

what Add variables to enable the Origin Shield for the CloudFront distribution why Using Origin Shield can help reduce the load on your origin. references https://registry.terraform.io/provide

Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)
feat: add origin-shield by syphernl · Pull Request #207 · cloudposse/terraform-aws-cloudfront-s3-cdnattachment image

what Add variables to enable the Origin Shield for the CloudFront distribution why Using Origin Shield can help reduce the load on your origin. references https://registry.terraform.io/provide

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)
feat: add origin-shield by syphernl · Pull Request #207 · cloudposse/terraform-aws-cloudfront-s3-cdnattachment image

what Add variables to enable the Origin Shield for the CloudFront distribution why Using Origin Shield can help reduce the load on your origin. references https://registry.terraform.io/provide

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

We are trying to prevent breaking the interface and without the default(...) function we can’t solve it. We are avoiding the experimental features due to the big disclaimer that the state object may change/break.

2022-01-18

2022-01-25

Frank avatar

Turns out the fix I made earlier does not work in all cases. This one should deal with that. https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/213

fix: handle viewer_certificate.ssl_support_method with CF default certificate by syphernl · Pull Request #213 · cloudposse/terraform-aws-cloudfront-s3-cdnattachment image

what Sets ssl_support_method to null when default certificate is being used instead of setting it to sni-only in all cases. why Setting this to an explicit value (sni-only) in conjunction with t…

1
Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)

See comments

fix: handle viewer_certificate.ssl_support_method with CF default certificate by syphernl · Pull Request #213 · cloudposse/terraform-aws-cloudfront-s3-cdnattachment image

what Sets ssl_support_method to null when default certificate is being used instead of setting it to sni-only in all cases. why Setting this to an explicit value (sni-only) in conjunction with t…

Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)

Approved

Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)

Going to release now as a hotfix.

Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)

@Frank I’m wrestling with Terratest here a bit. Will merge once I get tests to pass.

Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)

merged

Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)
[main.tf] Updated conditions with included cache_policy_id variable by eboboshka · Pull Request #210 · cloudposse/terraform-aws-cloudfront-s3-cdnattachment image

what Fixed conditions when using cache_policy_id why When using cache_policy_id, the module tries to change ttl values from zero to the default values. ~ default_cache_behavior { …

2022-01-26

2022-01-27

David Lozano avatar
David Lozano

Hi everyone. I’m a little bit confused with the bridgecrew’s comments on the below PR. Do I need to validate that the new alias records are pointing to resources created in my account? I didn’t see this validation in the repo before my changes.

Are these crewbridge’s comments the reason because the PR is blocked?

https://github.com/cloudposse/terraform-aws-route53-alias/pull/37

create records using for_each instead of count by 1david5 · Pull Request #37 · cloudposse/terraform-aws-route53-aliasattachment image

what Modify default and ipv6 aws_route53_record resources to use for_each instead of count. why Prevent destroying and recreating DNS records when removing elements from aliases list.

1
Yonatan Koren (Codefresh) avatar
Yonatan Koren (Codefresh)

Ah yes I took care of these for you by adding supressions. BridgeCrew’s policies can’t calculate anything that’s derived from a variable (outside of the root module I suppose?).

So the scenario is:

BridgeCrew: “I need to make sure that all of the alias records are pointing to resources in the same account as the alias”

Module: has general variable that allows for anything to be passed in

BridgeCrew: freaks out

create records using for_each instead of count by 1david5 · Pull Request #37 · cloudposse/terraform-aws-route53-aliasattachment image

what Modify default and ipv6 aws_route53_record resources to use for_each instead of count. why Prevent destroying and recreating DNS records when removing elements from aliases list.

David Lozano avatar
David Lozano

Ahh, got it. Thank you for reviewing

2022-01-30

    keyboard_arrow_up