#pr-reviews (2021-04)
Pull Request Reviews for Cloud Posse Projects
2021-04-05
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Can this one be reviewed (again) please? https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/147
what Replaced the enabled flag with distribution_enabled Corrected behavior for actual enabled flag to prevent from creating any resources why The enabled input was being used to manage the dist…
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@jose.amengual
what Replaced the enabled flag with distribution_enabled Corrected behavior for actual enabled flag to prevent from creating any resources why The enabled input was being used to manage the dist…
![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)
@Frank there is issues with your PR, it looks like the logic on the count is not correct, I left a message on the PR merged and tagged you on the Bug, if you can’t figure out please let me know and I will revert the change
![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)
@Frank I saw the replies to the bug, thanks
2021-04-06
2021-04-08
2021-04-14
![rei avatar](https://secure.gravatar.com/avatar/707f916d5733af8f0ce7938695a8da03.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0005-72.png)
Can you review this PR please: https://github.com/cloudposse/terraform-aws-eks-node-group/pull/65
what Add placement variable in order to specify optional placement parameters for the managed nodes (e.g. pin them to a single AZ). When the placement variable is set, the module checks if an avai…
2021-04-19
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Can someone please take a look at this one? https://github.com/cloudposse/terraform-aws-dynamic-subnets/pull/131
what Replaced removed list function with tolist to make it work with Terraform 0.15. Updated Github Workflows why The list function was deprecated since Terraform v0.12 and has been removed in T…
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
I’ll resurface with the contributor team to get an admin approval on this one.
what Replaced removed list function with tolist to make it work with Terraform 0.15. Updated Github Workflows why The list function was deprecated since Terraform v0.12 and has been removed in T…
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
@Frank merged
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Thanks!
![Matthew Tovbin avatar](https://avatars.slack-edge.com/2021-04-14/1964986697778_07a79233626ede4c0442_72.jpg)
Hi folks, who is in charge of reviewing the PRs on cloudposse / terraform-aws-rds-cloudwatch-sns-alarms ?
Terraform module that configures important RDS alerts using CloudWatch and sends them to an SNS topic - cloudposse/terraform-aws-rds-cloudwatch-sns-alarms
2021-04-21
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
what synchronizing part of codebase to match terraform-aws-alb access_logs_region is no longer used bumping vpc and subnet versions to the latest releases for testing syncing access_logs to match …
2021-04-22
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
not sure how to rerun the failed bridgecrew test
what Describe high-level what changed as a result of these commits (i.e. in plain-english, what do these changes mean?) Use bullet points to be concise and to the point. Context, as inherited fr…
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
Can you share the output of the failed test?
what Describe high-level what changed as a result of these commits (i.e. in plain-english, what do these changes mean?) Use bullet points to be concise and to the point. Context, as inherited fr…
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
i cannot see it
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
ah, i’ve talked to you a few times on here and assumed you were affiliated with CP. my bad haha
![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)
I will do it
![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)
Ensure the ELBv2 (Application/Network) has access logging enabled
![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)
Ensure that ALB drops HTTP headers
![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)
this is because bridgecrew thinks they need to be enable but we have a var to enable or disable it
![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)
it expect it to be enabled
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
@jose.amengual and other CloudPosse staff, is there a way to expose what needs to be fixed through bridgecrew? I’m guessing their TOS won’t allow that though, not sure. Alternatively, do you have any opensource ambassadors that have read access to the BridgeCrew results? With the latest updates to Checkov you’ll probably need some help with these broken builds.
![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)
not yet, we do not have a way to do that so far
![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)
I think for this we need to anotate exception rules
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
Yeah, I’m famiar with this part of the process. Just curious about the best way to make this part of the workflow a bit more automated and to make it so that users can either find the results or replicate the process on their side. The quick and easy solution is documenting something like this https://github.com/bridgecrewio/checkov#using-docker so that the user can do it themselves.
Prevent cloud misconfigurations during build-time for Terraform, Cloudformation, Kubernetes, Serverless framework and other infrastructure-as-code-languages with Checkov by Bridgecrew. - bridgecrew…
![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)
They said they were working on a solution for this but I have not heard back
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
Any chance we could we get another PR review on this one?
![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)
anyone know why auto format fails like this? https://github.com/cloudposse/terraform-aws-cicd/pull/94/checks
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
@mihai.plesa looks to be an issue with there not being a related
entry in the README.yaml
. Check out other README.yaml
files and copy the related key from them and remove any elements and you should be good.
![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)
thank you Matt, that worked. just needs review now
![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)
and someone to start the tests
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
Started the tests, but asked a question as I’m not familiar with this module. Someone else from the contributor team might be more helpful.
![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)
I’m testing now without passing website_bucket_name
![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)
@Matt Gowie please have a look now, it’s ready for tests again
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
@mihai.plesa 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)
awesome, thank you
![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)
![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)
lol
![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)
requesting reviews on the following - please https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/61 https://github.com/cloudposse/terraform-aws-cicd/pull/94
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
another PR, please https://github.com/cloudposse/terraform-aws-lb-s3-bucket/pull/38
what All load balancer types will be able to log to the s3 bucket. why The missing policy info prevents logging from all load balancer types and is incomplete according to AWS documentation. re…
2021-04-23
![MattyB avatar](https://secure.gravatar.com/avatar/ff034363a31c46cbb9df6b6b2a8c82ae.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
https://github.com/cloudposse/terraform-aws-nlb/pull/13 https://github.com/cloudposse/terraform-aws-alb/pull/86
what synchronizing part of codebase to match terraform-aws-alb access_logs_region is no longer used bumping vpc and subnet versions to the latest releases for testing syncing access_logs to match …
what Describe high-level what changed as a result of these commits (i.e. in plain-english, what do these changes mean?) Use bullet points to be concise and to the point. Context, as inherited fr…
2021-04-26
![Harry Macey avatar](https://avatars.slack-edge.com/2021-04-26/2001696155602_c98ac60d32509c6efc0f_72.png)
what Adding the ability to add multiple origins Duplicated from: cloudposse/terraform-aws-cloudfront-s3-cdn#78 why Allow more origins to be referenced in the ordered_cache block.
![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)
thanks @jose.amengual
what Adding the ability to add multiple origins Duplicated from: cloudposse/terraform-aws-cloudfront-s3-cdn#78 why Allow more origins to be referenced in the ordered_cache block.
![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)
![Leo Zavala avatar](https://secure.gravatar.com/avatar/4e2af4492d3596b382d555013a4c882d.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0020-72.png)
Would have a look at this PR? - please Not sure what to do about the BC Scanner…
https://github.com/cloudposse/terraform-aws-cloudwatch-flow-logs/pull/16
What Add GitHub Actions Updates Readme Upgrade to TF13/14 Adds example/complete Adds Test Why Attempting to add automation. Attempting to upgrade module to TF 14 References Close #3 & #6
![Harry Macey avatar](https://avatars.slack-edge.com/2021-04-26/2001696155602_c98ac60d32509c6efc0f_72.png)
https://github.com/cloudposse/terraform-aws-s3-bucket/pull/86 One-liner bug fix Thank you in advance!
what Fixing a bug where the bucket policy would flip-flop on Terraform apply if var.policy and any of var.allow_ssl_requests_only, var.allow_encrypted_uploads_only were set why The aws_s3_bucket…
2021-04-29
![Joe Niland avatar](https://secure.gravatar.com/avatar/b90c8e752dd648ef229096c60ba2408f.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0022-72.png)
https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/146 - review please
what Modify build_environment_variables var type to include a 'type' map key. why CodePipeline and CodeBuild modules consumed by this module now require the type key to be specified. re…