#pr-reviews (2021-12)
Pull Request Reviews for Cloud Posse Projects
2021-12-02
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
looks like tests passed, please review
@Jeremy G (Cloud Posse) there’s a comment regarding breaking change from you, any objections to upgrade now?
@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.
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.
@mihai.plesa please remove provider altogether, pick the strategy you like try it. We will post it as a migration note in the release
Jeremy provided same in the PR
2021-12-03
this prevented us from upgrading the ALB module and is a 10s review, thanks https://github.com/cloudposse/terraform-aws-alb/pull/108
2021-12-07
2021-12-08
2021-12-10
tiny one char change, plz review https://github.com/cloudposse/terraform-aws-rds-cluster/pull/128
Did you check the reasoning behind prev change? You might need a debate
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
It was the previous logic and was an error in the last PR
See the previous PR linked in the summary of the PR 128
I thought there was someone who changed exactly this previously
@Max Lobur (Cloud Posse) ah ok, could you review the pr then ?
2021-12-22
anyone around to unblock this? https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/78#issuecomment-999059522
are you talking about the comment or the bridgecrew warnings?
@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
for bridecrew you need to add exceptions at the top of the file
yep
added
the nitrocode comment is not resolved?
merged