SweetOps #pr-reviews for May, 2024

Pull Request Reviews for Cloud Posse Projects

2024-05-01

Frank

10:06:09 AM

https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/227

I added support for Serverless Redis to this module

#227 feat: add support for redis serverless

what

• Add support for Serverless Redis instances

why

• Supporting Redis Serverless for demanding applications

notes

• This upgrades the required version of the AWS provider from “>= 4.18” to “>= 5.32” as this is the first release where aws_elasticache_serverless_cache was introduced

2024-05-02

Ben

04:49:46 PM

Hey there, can someone have a look at this PR discussion? https://github.com/cloudposse/terraform-aws-lb-s3-bucket/pull/82#issuecomment-2090913789 It’s about the correct use of the null label module to generate a default bucket name.

Fix bucket name in policy statements, fixing #81 by benjamin-hg · Pull Request #82 · cloudposse/terraform-aws-lb-s3-bucket attachment image

references

closes #81 Copy bucket_name logic from dependent module

Duleendra

02:46:28 AM

Hi Team, Gret help if you guys can help to review this PR https://github.com/cloudposse/terraform-aws-waf/pull/57

fix content_type in aws_wafv2_web_acl to use correct map value by hostekevin · Pull Request #57 · cloudposse/terraform-aws-waf attachment image

what Fixed a typo in aws_wafv2_web_acl resource: changed content_type assignment. why Corrects content_type mapping to use the appropriate value. references Minor fix, no related GitHub issue.

05:02:07 AM

Looks like the readme needs to be updated.

Try running

make init
make docs
make readme

and pushing up

fix content_type in aws_wafv2_web_acl to use correct map value by hostekevin · Pull Request #57 · cloudposse/terraform-aws-waf attachment image

what Fixed a typo in aws_wafv2_web_acl resource: changed content_type assignment. why Corrects content_type mapping to use the appropriate value. references Minor fix, no related GitHub issue.

Duleendra

06:44:51 AM

Actually I am not the author of the PR. I wanted to get the attention for the PR because we are desperately waiting this fix as we depend on WAF module for prod workloads

2024-05-03

04:58:15 AM

Very small PR (one-liner). Adding a try() around a length() function to stop it from throwing an error.

https://github.com/cloudposse/terraform-aws-rds/pull/179

Michael

03:27:15 AM

Good catch and thanks for providing the use case in the description

2024-05-05

2024-05-06

Kamil Grabowski

01:37:00 PM

Could you please review this pull request: https://github.com/terraform-aws-modules/terraform-aws-acm/pull/149 It would be nice to have this feature merged

#149 feat: Allow multiple domains in a single certificate

This is a follow up of #137
Credits to @amontalban for the work, I made requested changes and tested the feature.

Description

This PR allows creating one ACM certificate for multiple domains, which, is useful when using the certificate for CloudFront that only allows one certificate per distribution.

Motivation and Context

CloudFront does not support multiple ACM certificates, like ALB. Therefore, if you need to support multiple domains in a single CloudFront distribution you would have to create the certificate manually because this module does not support it.

Breaking Changes

This change should be backward compatible as I added a zones var containing a map with domains and their Route53 zone ID so the validation records are created in the correct Route53 zone.

Additionally, I have updated the tests in examples/complete-dns-validation to allow variables so it was easier for me (and others) to test with my test domains.

How Has This Been Tested?

☑︎ I have updated at least one of the examples/* to demonstrate and validate my change(s) ☑︎ I have tested and validated these changes using one or more of the provided examples/* projects ☑︎ I have executed pre-commit run -a on my pull request

Fixes #136

2024-05-07

01:30:07 PM

In PR https://github.com/cloudposse/terraform-aws-ec2-bastion-server/pull/230/files

• The tflint step is failing here due to a github token issue • The codeowners are set to admins Is this repo out of date?

Erik Osterman (Cloud Posse)

10:24:06 PM

We’re rolling out the fix as we speak

Erik Osterman (Cloud Posse)

10:24:48 PM

https://github.com/search?q=org%3Acloudposse%20is%3Apr%20is%3Aclosed%20Update%20Gi[…]s%20to%20Fix%20ReviewDog%20TFLint%20Action%20&type=pullrequests

Ray Finch

08:11:38 PM

Hi, could I get the PR reviewed? https://github.com/cloudposse/terraform-aws-rds-cluster/pull/192

#192 implement create_before_destroy on instances

what

I implemented create_before_destroy on the aws_rds_cluster_instance default instances.

why

Making a change to any parameter that triggers a replace on a aws_rds_cluster_instance results in all instances being destroyed before attempting to create a new instance which causes an outage. This a faster (and safer) altenative to #191

references

This closes #190 and is an alternative to #191

Ercan Ermis

09:17:48 PM

Hello all. could you check my PR? ref: https://github.com/cloudposse/terraform-aws-elasticache-memcached/pull/79

2024-05-10

Ercan Ermis

08:02:46 PM

Hello, I need a PR review. Thanks ref: https://github.com/cloudposse/terraform-aws-ec2-instance/pull/197

2024-05-14

z0rc3r

10:19:35 AM

Could you take a look please? https://github.com/cloudposse/terraform-aws-eks-cluster/pull/227

#227 Add Service IPv4 CIDR to output

what

Implements #226.

why

See linked issue.

references

See linked issue.

2024-05-15

12:42:16 AM

Could i get some help with the failing checks in this pr ?

https://github.com/cloudposse/terraform-aws-components/pull/1032

#1032 feat(vpc): add named subnets

what

• add named subnets to vpc component

why

• allow using named subnets for databases

references

• https://sweetops.slack.com/archives/C031919U8A0/p1715359642738549?thread_ts=1715359642.738549&cid=C031919U8A0

12:42:39 AM

@RB we’d appreciate it if you opened a PR and add it

Dan Miller (Cloud Posse)

10:09:49 PM

Thanks for surfacing this. I’m looking into it internally and will let you know once it’s resolved

Dan Miller (Cloud Posse)

01:54:56 AM

This repo isn’t supposed to have those failing workflows. We’re removing them once Erik confirms this PR https://github.com/cloudposse/terraform-aws-components/pull/1038

#1038 fix: Remove `feature-branch` GitHub Actions workflow

what

• Deleted the feature-branch.yaml workflow

why

• This repo should not have this workflow. It’s intended for module, not component, repos.

references

• Internal Slack thread

2024-05-16

Ercan Ermis

02:32:21 AM

hello all, new PR is here I’m eating https://github.com/cloudposse/terraform-aws-rds/pull/181 thank you for your time and effort! best!

#181 add(outputs): paramter_group_name and option_group_name

what

Output values.

why

We need to pass in the rds.

references

n/a

Ercan Ermis

02:32:39 AM

@Ozan Gazi cc

#181 add(outputs): paramter_group_name and option_group_name

what

Output values.

why

We need to pass in the rds.

references

n/a

Andriy Knysh (Cloud Posse)

02:39:48 AM

thanks @Ercan Ermis the PR looks good, please address the comments and we’ll merge it

Ercan Ermis

06:46:17 PM

hello @Andriy Knysh (Cloud Posse), they are completed and ready to merge. thanks! ^^

Joe Niland

04:17:45 AM

Could I please get a review on https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/238 ?

#238 Bump AWS provider and update examples to include ECS Service Connect config

what

• Update examples/complete to include ECS Service Connect config • bump AWS provider version to support latest ECS Service Connect config

why

• Fix issue when using older AWS provider • Increase test coverage

references

• Closes #236

2024-05-17

2024-05-18

Duleendra

02:59:54 PM

Hi Team, help to review this https://github.com/cloudposse/terraform-aws-waf/pull/79 . While using the WAF module we noticed this part is missing and just thought to raise the PR. Thank you

#79 feat: add a custom response body for the default block action

what

Add a response body for the default blocked action by choosing from the existing custom response bodies.

why

Sometimes, users may want to display a custom response message for default blocked action.

references