#pr-reviews (2021-12)

Pull Request Reviews for Cloud Posse Projects

2021-12-23

2021-12-22

mihai avatar
mihai
1
jose.amengual avatar
jose.amengual

are you talking about the comment or the bridgecrew warnings?

mihai avatar
mihai

@’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
jose.amengual

for bridecrew you need to add exceptions at the top of the file

jose.amengual avatar
jose.amengual

yep

mihai avatar
mihai

added

jose.amengual avatar
jose.amengual

the nitrocode comment is not resolved?

jose.amengual avatar
jose.amengual

merged

mihai avatar
mihai

many thanks

1

2021-12-10

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)
Create dns record if not serverless by nitrocode · Pull Request #128 · cloudposse/terraform-aws-rds-cluster attachment image

what Restore original logic why Previous logic was to create the record when module was not serverless references Previous PR #124

1
Max Lobur (Cloud Posse) avatar
Max Lobur (Cloud Posse)

Did you check the reasoning behind prev change? You might need a debate

Create dns record if not serverless by nitrocode · Pull Request #128 · cloudposse/terraform-aws-rds-cluster attachment image

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
Max Lobur (Cloud Posse)
Fix: ordered_cache_behavior forwarded_values dynamic block by neilbartley · Pull Request #188 · cloudposse/terraform-aws-cloudfront-s3-cdn attachment image

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 (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

It was the previous logic and was an error in the last PR

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

See the previous PR linked in the summary of the PR 128

Max Lobur (Cloud Posse) avatar
Max Lobur (Cloud Posse)
fix: prevent creating empty replicas record by syphernl · Pull Request #126 · cloudposse/terraform-aws-rds-cluster attachment image

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
Max Lobur (Cloud Posse)

I thought there was someone who changed exactly this previously

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

@ ah ok, could you review the pr then ?

Max Lobur (Cloud Posse) avatar
Max Lobur (Cloud Posse)

np approved

1

2021-12-08

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)
Added evaluation periods in description by nitrocode · Pull Request #74 · cloudposse/terraform-aws-ec2-autoscale-group attachment image

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…

1
1

2021-12-07

2021-12-03

mihai avatar
mihai

this prevented us from upgrading the ALB module and is a 10s review, thanks https://github.com/cloudposse/terraform-aws-alb/pull/108

Fix default target group count by linhkikuchi · Pull Request #108 · cloudposse/terraform-aws-alb attachment image

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…

1
1

2021-12-02

mihai avatar
mihai
feat: add origin_request_policy_id variable for default cache behavior by bartelemi · Pull Request #193 · cloudposse/terraform-aws-cloudfront-s3-cdn attachment image

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

1
1
mihai avatar
mihai

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

1
Max Lobur (Cloud Posse) avatar
Max Lobur (Cloud Posse)

@Jeremy (Cloud Posse) there’s a comment regarding breaking change from you, any objections to upgrade now?

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@ @ 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
Max Lobur (Cloud Posse)

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
Max Lobur (Cloud Posse)

@ 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
Max Lobur (Cloud Posse)

Jeremy provided same in the PR

    keyboard_arrow_up