#pr-reviews (2024-05)

Pull Request Reviews for Cloud Posse Projects

2024-05-01

Frank avatar

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 avatar

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.

1
1
Duleendra Shashimal avatar
Duleendra Shashimal

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-wafattachment 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.

RB avatar

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-wafattachment 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 Shashimal avatar
Duleendra Shashimal

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

RB avatar

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

1
Michael avatar
Michael

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

1

2024-05-05

2024-05-06

Kamil Grabowski avatar
Kamil Grabowski

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

RB avatar

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?

Ray Finch avatar
Ray Finch
#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

2024-05-10

Ercan Ermis avatar
Ercan Ermis
1

2024-05-14

z0rc3r avatar
#227 Add Service IPv4 CIDR to output

what

Implements #226.

why

See linked issue.

references

See linked issue.

1

2024-05-15

RB avatar

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

1
Dan Miller (Cloud Posse) avatar
Dan Miller (Cloud Posse)

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

1
Dan Miller (Cloud Posse) avatar
Dan Miller (Cloud Posse)

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

1
1

2024-05-16

Ercan Ermis avatar
Ercan Ermis

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

1
Ercan Ermis avatar
Ercan Ermis

@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) avatar
Andriy Knysh (Cloud Posse)

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

1
Ercan Ermis avatar
Ercan Ermis

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

Joe Niland avatar
Joe Niland
#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

1

2024-05-17

    keyboard_arrow_up