#pr-reviews (2023-06)
Pull Request Reviews for Cloud Posse Projects
2023-06-01
![Thomas Poetke avatar](https://avatars.slack-edge.com/2023-07-03/5520481095762_ce020f3f026c08394e2b_72.jpg)
I have an option to the ALB module added: https://github.com/cloudposse/terraform-aws-alb/pull/143
what
add xff_header_processing_mode option to alb
why
option is missing
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Thomas Poetke we’ll review it
what
add xff_header_processing_mode option to alb
why
option is missing
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Thomas Poetke there are comments to be addressed. Pls check
![Thomas Poetke avatar](https://avatars.slack-edge.com/2023-07-03/5520481095762_ce020f3f026c08394e2b_72.jpg)
I did, but precommit isnt working for me
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Max Lobur (Cloud Posse) any thoughts?
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
![Max Lobur (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-07-20/2316891735296_3098d8d2760936592f52_72.jpg)
The errors you have are docs - this will re-generate you docs
![Thomas Poetke avatar](https://avatars.slack-edge.com/2023-07-03/5520481095762_ce020f3f026c08394e2b_72.jpg)
I resynced the fork, after https://github.com/cloudposse/terraform-aws-alb/pull/142 is merged , I will retry to do the pre commit
what
Support AWS Provider V5
Linter fixes
why
Maintenance
references
https://github.com/hashicorp/terraform-provider-aws/releases/tag/v5.0.0
![Thomas Poetke avatar](https://avatars.slack-edge.com/2023-07-03/5520481095762_ce020f3f026c08394e2b_72.jpg)
2023-06-02
![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)
what
Add the ability to add external ENIs to the instance
why
For ec2 instances deployments clusters where the IPs are required to be unique and not change it is necessary to create ENIs outside of this module so that the instance termination does not change the ENI and IP attached.
references
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Max Lobur (Cloud Posse) @Andriy Knysh (Cloud Posse)
what
Add the ability to add external ENIs to the instance
why
For ec2 instances deployments clusters where the IPs are required to be unique and not change it is necessary to create ENIs outside of this module so that the instance termination does not change the ENI and IP attached.
references
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@jose.amengual we’ll review 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)
thanks
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
@jose.amengual thank you for the PR, looks good. I left a few comments
![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 address them , Thanks!
2023-06-05
2023-06-06
2023-06-07
2023-06-08
2023-06-09
2023-06-10
2023-06-11
2023-06-12
2023-06-13
2023-06-14
2023-06-15
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
Can someone review this PR? We found this is a valid one but not merged, https://github.com/cloudposse/terraform-aws-config/pull/44. Thanks!
![attachment image](https://user-images.githubusercontent.com/22661159/146816907-d6f62bac-9a20-40bc-a32a-fc425aa27877.png)
what
Adds a valid key of type string for sns subscriber handler
why
A recent commit adds defaults to subscriber input but also breaks any input on map key type:
image
Proposed fix tested with following input, pulled from cloudposse/terraform-aws-sns-topic:
locals {
subscribers = {
opsgenie = {
protocol = "https"
endpoint = "<https://api.example.com/v1/>"
endpoint_auto_confirms = true
}
}
}
module "test" {
source = "../terraform-aws-config"
# Cloud Posse recommends pinning every module to a specific version
# version = "x.x.x"
create_sns_topic = true
create_iam_role = true
subscribers = local.subscribers
}
thanks
Thank you for these modules!
![Linda Pham (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-04-13/3374321180711_438a91ddd259ebac3590_72.jpg)
@Hao Wang will have someone review
![attachment image](https://user-images.githubusercontent.com/22661159/146816907-d6f62bac-9a20-40bc-a32a-fc425aa27877.png)
what
Adds a valid key of type string for sns subscriber handler
why
A recent commit adds defaults to subscriber input but also breaks any input on map key type:
image
Proposed fix tested with following input, pulled from cloudposse/terraform-aws-sns-topic:
locals {
subscribers = {
opsgenie = {
protocol = "https"
endpoint = "<https://api.example.com/v1/>"
endpoint_auto_confirms = true
}
}
}
module "test" {
source = "../terraform-aws-config"
# Cloud Posse recommends pinning every module to a specific version
# version = "x.x.x"
create_sns_topic = true
create_iam_role = true
subscribers = local.subscribers
}
thanks
Thank you for these modules!
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
cool, thanks
![Linda Pham (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-04-13/3374321180711_438a91ddd259ebac3590_72.jpg)
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
Thanks, @James A ^^^
2023-06-16
2023-06-19
2023-06-20
2023-06-21
2023-06-22
2023-06-25
![Aleksandar Knezevic avatar](https://avatars.slack-edge.com/2021-07-29/2338445254929_ae0580e50070ef9c79b0_72.jpg)
what
• Introduces using cloudposse s3-bucket module for creating ALB logging bucket • Introducing new variable which defines if s3 logs for ALB is enabled or disabled (default is still true) • Adding random suffix to the name of logging bucket (since names of S3 must be globally unique - very often name provided in module is in collision with some already existed) • If created, S3 bucket for storing ALB access logs is encrypted by default
why
• This module not usable anymore due to deprecation message by terraform about s3 bucket (see #227 ) • Users of this module don’t always need access logs from Load Balancer n S3 bucket
references
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
maybe instead of S3 bucket module you should use this module https://github.com/cloudposse/terraform-aws-lb-s3-bucket
Terraform module to provision an S3 bucket with built in IAM policy to allow AWS Load Balancers to ship access logs
![Aleksandar Knezevic avatar](https://avatars.slack-edge.com/2021-07-29/2338445254929_ae0580e50070ef9c79b0_72.jpg)
@kevcube Thank you for suggestion. PR is updated
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Aleksandar Knezevic Please see the new comments by Amdriy
![Aleksandar Knezevic avatar](https://avatars.slack-edge.com/2021-07-29/2338445254929_ae0580e50070ef9c79b0_72.jpg)
Thank you @Gabriela Campana (Cloud Posse) . PR is updated and ready for review.
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Andriy Knysh (Cloud Posse)
2023-06-26
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
https://github.com/cloudposse/terraform-aws-lb-s3-bucket/pull/69
Ideally this can be rebased to 1.4.2
THEN please update this PR to the latest version of lb-s3-bucket (likely 0.19) https://github.com/cloudposse/terraform-aws-alb/pull/139
then finally I will update my PR and it can be merged https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/189
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
Working on these. Ping me if I don’t follow up