SweetOps #pr-reviews for January, 2022

Pull Request Reviews for Cloud Posse Projects

2022-01-11

Frank

Can someone take a look at https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/208 & https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/207?

fix: set viewer_certificate.ssl_support_method to a non-empty value by syphernl · Pull Request #208 · cloudposse/terraform-aws-cloudfront-s3-cdn attachment 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-cdn attachment 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

06:26:02 PM

Just waiting on https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/207#issuecomment-1012253728

@Erik Osterman (Cloud Posse)

feat: add origin-shield by syphernl · Pull Request #207 · cloudposse/terraform-aws-cloudfront-s3-cdn attachment 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)

08:23:50 PM

Really it comes down to https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/207#discussion_r784115283

feat: add origin-shield by syphernl · Pull Request #207 · cloudposse/terraform-aws-cloudfront-s3-cdn attachment 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)

08:24:43 PM

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

09:43:18 AM

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-cdn attachment 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

07:12:25 PM

See comments

fix: handle viewer_certificate.ssl_support_method with CF default certificate by syphernl · Pull Request #213 · cloudposse/terraform-aws-cloudfront-s3-cdn attachment 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

07:23:37 PM

Approved

Yonatan Koren

07:23:46 PM

Going to release now as a hotfix.

Yonatan Koren

04:25:36 PM

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

Yonatan Koren

05:16:14 PM

merged

Yonatan Koren

05:16:26 PM

Going to try and release it with https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/210

[main.tf] Updated conditions with included cache_policy_id variable by eboboshka · Pull Request #210 · cloudposse/terraform-aws-cloudfront-s3-cdn attachment 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

05:35:50 PM

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-alias attachment 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.

Yonatan Koren

05:13:57 PM

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-alias attachment 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

05:43:38 PM

Ahh, got it. Thank you for reviewing

#pr-reviews (2022-01)

2022-01-11

2022-01-18

2022-01-25

2022-01-26

2022-01-27

2022-01-30