#pr-reviews (2021-12)
Pull Request Reviews for Cloud Posse Projects
2021-12-02
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
quick one that has been waiting for a bit https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/193
what Add origin_request_policy_id input variable for setting this param in default cache behavior. why This param is missing from the module. references https://registry.terraform.io/providers/has…
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
if someone can enable the test runs, need this in to be able to apply from M1 macs https://github.com/cloudposse/terraform-github-repository-webhooks/pull/35
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
looks like tests passed, please review
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
@Jeremy G (Cloud Posse) there’s a comment regarding breaking change from you, any objections to upgrade now?
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
@Max Lobur (Cloud Posse) @mihai.plesa I think we should remove the provider altogether. In any case, this is going to be a painful upgrade for anyone already using this module, so we need to publish a migration document.
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
We’ve had discussions internally, proposed migration strategies for removing provider are
• Easiest: destroy the current webhooks, upgrade the root module to supply a github provider and upgrade to the new version of this module, then create new webhooks.
• Next easiest, and probably works: remove the webhooks from the current Terraform state with terraform state remove, upgrade the root module to supply a github provider and upgrade to the new version of this module, then import the old webhooks into the new state.
• Harder, and probably no better than the previous technique, follow the guidance in provider issue #652 to directly migrate the provider and webhook resource.
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
@mihai.plesa please remove provider altogether, pick the strategy you like try it. We will post it as a migration note in the release
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
Jeremy provided same in the PR
2021-12-03
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
this prevented us from upgrading the ALB module and is a 10s review, thanks https://github.com/cloudposse/terraform-aws-alb/pull/108
what I have listener_https_fixed_response but still need the default target group created in this module. It used to work before this PR (#97). Do not see why it should not get created when we hav…
2021-12-07
2021-12-08
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
what Added evaluation periods in description why Otherwise the description would show that it would only take X seconds to scale up/down which is incorrect when the evaluation periods is greater…
2021-12-10
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
tiny one char change, plz review https://github.com/cloudposse/terraform-aws-rds-cluster/pull/128
what Restore original logic why Previous logic was to create the record when module was not serverless references Previous PR #124
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
Did you check the reasoning behind prev change? You might need a debate
what Restore original logic why Previous logic was to create the record when module was not serverless references Previous PR #124
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
Similar to https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/188#issuecomment-989721183 I propose to move it to a separate boolean var
what Corrects logic for ordered_cache_behavior -> forwarded_values Should be If a cache policy or origin request policy is specified, we cannot include a 'forwarded_values' block at all…
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
It was the previous logic and was an error in the last PR
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
See the previous PR linked in the summary of the PR 128
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
ah pardon, I messed with https://github.com/cloudposse/terraform-aws-rds-cluster/pull/126/files
what Prevent creating empty DNS replicas record when cluster_size < 1 why If the cluster_size = 0 this would result in an attempt to create an empty DNS record, which is not permitted by the …
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
I thought there was someone who changed exactly this previously
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
@Max Lobur (Cloud Posse) ah ok, could you review the pr then ?
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
2021-12-22
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
anyone around to unblock this? https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/78#issuecomment-999059522
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
are you talking about the comment or the bridgecrew warnings?
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
@Harry Macey’s comment is about the BridgeCrew warning - wonder if WAF is really needed or how we can ignore that rule to get this merged and released
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
for bridecrew you need to add exceptions at the top of the file
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
yep
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
added
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
the nitrocode comment is not resolved?
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
merged
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)