#pr-reviews (2021-04)

Pull Request Reviews for Cloud Posse Projects

2021-04-29

Joe Niland avatar
Joe Niland
Support 'type' key in Codebuild environment variables by joe-niland · Pull Request #146 · cloudposse/terraform-aws-ecs-web-app attachment image

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…

1

2021-04-26

Harry Macey avatar
Harry Macey
Adding custom_origins variable by justnom · Pull Request #61 · cloudposse/terraform-aws-cloudfront-cdn attachment image

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.

1
1
1
mihai avatar
mihai

thanks @jose.amengual

Adding custom_origins variable by justnom · Pull Request #61 · cloudposse/terraform-aws-cloudfront-cdn attachment image

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

np

1
Leo Zavala avatar
Leo Zavala

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

Bring repo up to standard by lezavala · Pull Request #16 · cloudposse/terraform-aws-cloudwatch-flow-logs attachment image

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
Harry Macey

https://github.com/cloudposse/terraform-aws-s3-bucket/pull/86 One-liner bug fix Thank you in advance!

Removing policy attribute for S3 bucket by justnom · Pull Request #86 · cloudposse/terraform-aws-s3-bucket attachment image

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…

1

2021-04-23

MattyB avatar
MattyB
reintroduce support for access logs by wmborelli · Pull Request #13 · cloudposse/terraform-aws-nlb attachment image

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 …

Pass module.this.context to cloudposse/lb-s3-bucket/aws by jwstric2 · Pull Request #86 · cloudposse/terraform-aws-alb attachment image

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-22

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

not sure how to rerun the failed bridgecrew test

https://github.com/cloudposse/terraform-aws-alb/pull/86

Pass module.this.context to cloudposse/lb-s3-bucket/aws by jwstric2 · Pull Request #86 · cloudposse/terraform-aws-alb attachment image

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…

1
MattyB avatar
MattyB

Can you share the output of the failed test?

Pass module.this.context to cloudposse/lb-s3-bucket/aws by jwstric2 · Pull Request #86 · cloudposse/terraform-aws-alb attachment image

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

i cannot see it

MattyB avatar
MattyB

ah, i’ve talked to you a few times on here and assumed you were affiliated with CP. my bad haha

jose.amengual avatar
jose.amengual

I will do it

jose.amengual avatar
jose.amengual
	Ensure the ELBv2 (Application/Network) has access logging enabled
jose.amengual avatar
jose.amengual
	Ensure that ALB drops HTTP headers
jose.amengual avatar
jose.amengual

this is because bridgecrew thinks they need to be enable but we have a var to enable or disable it

jose.amengual avatar
jose.amengual

it expect it to be enabled

MattyB avatar
MattyB

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

not yet, we do not have a way to do that so far

jose.amengual avatar
jose.amengual

I think for this we need to anotate exception rules

MattyB avatar
MattyB

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.

bridgecrewio/checkov attachment image

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

They said they were working on a solution for this but I have not heard back

MattyB avatar
MattyB

Any chance we could we get another PR review on this one?

jose.amengual avatar
jose.amengual

merged

mihai avatar
mihai

anyone know why auto format fails like this? https://github.com/cloudposse/terraform-aws-cicd/pull/94/checks

Matt Gowie avatar
Matt Gowie

@ 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 avatar
mihai

thank you Matt, that worked. just needs review now

mihai avatar
mihai

and someone to start the tests

Matt Gowie avatar
Matt Gowie

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 avatar
mihai

I’m testing now without passing website_bucket_name

mihai avatar
mihai

@Matt Gowie please have a look now, it’s ready for tests again

Matt Gowie avatar
Matt Gowie

@ merged

mihai avatar
mihai

awesome, thank you

jose.amengual avatar
jose.amengual

wow that was a big picture

mihai avatar
mihai

not anymore

jose.amengual avatar
jose.amengual

lol

MattyB avatar
MattyB
update s3_policy to work with all lb types by wmborelli · Pull Request #38 · cloudposse/terraform-aws-lb-s3-bucket attachment image

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…

1

2021-04-21

MattyB avatar
MattyB
reintroduce support for access logs by wmborelli · Pull Request #13 · cloudposse/terraform-aws-nlb attachment image

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-19

Frank avatar
Frank
feat: add terraform 0.15 support and update github flows by syphernl · Pull Request #131 · cloudposse/terraform-aws-dynamic-subnets

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…

1
Matt Gowie avatar
Matt Gowie

I’ll resurface with the contributor team to get an admin approval on this one.

feat: add terraform 0.15 support and update github flows by syphernl · Pull Request #131 · cloudposse/terraform-aws-dynamic-subnets

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
Matt Gowie

@ merged

Frank avatar
Frank

Thanks!

Matthew Tovbin avatar
Matthew Tovbin
Hi folks, who is in charge of reviewing the PRs on [cloudposse>  /  <https://github.com/cloudposse/terraform-aws-rds-cloudwatch-sns-alarms terraform-aws-rds-cloudwatch-sns-alarms](https://github.com/cloudposse?type=source) ?
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-14

rei avatar
Allow for Node Group placement attributes by reixd · Pull Request #65 · cloudposse/terraform-aws-eks-node-group

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…

1
1

2021-04-08

2021-04-06

2021-04-05

Frank avatar
Frank
Correct enabled behavior by syphernl · Pull Request #147 · cloudposse/terraform-aws-cloudfront-s3-cdn

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…

1
1
Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@jose.amengual

Correct enabled behavior by syphernl · Pull Request #147 · cloudposse/terraform-aws-cloudfront-s3-cdn

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…

1
jose.amengual avatar
jose.amengual

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

@ I saw the replies to the bug, thanks

    keyboard_arrow_up