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

Duleendra

02:27:22 PM

Hi Team , help to review this 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

Gabriela Campana (Cloud Posse)

03:52:18 PM

Hi @Duleendra Just asked our team to review it

Duleendra

05:00:43 PM

Thanks @Gabriela Campana (Cloud Posse)

2024-05-22

04:49:08 AM

Could I get a PR review for overriding the dynamodb table name ?

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

#1016 feat: allow overriding the dynamodb table name

what

• allow overriding the dynamodb table name

why

• This will make it possible to import older dynamodb tables without having to null portions of the context • Matches s3-bucket module’s bucket_name

references

• See cloudposse/terraform-aws-dynamodb#126 • This depends on the 0.36.0 release https://github.com/cloudposse/terraform-aws-dynamodb/releases

Dan Miller (Cloud Posse)

12:12:47 AM

This is already approved. We usually let the submitter merge their own PR after it’s approved. Or you could enable auto-merge if you prefer to submit and forget it

#1016 feat: allow overriding the dynamodb table name

what

• allow overriding the dynamodb table name

why

• This will make it possible to import older dynamodb tables without having to null portions of the context • Matches s3-bucket module’s bucket_name

references

• See cloudposse/terraform-aws-dynamodb#126 • This depends on the 0.36.0 release https://github.com/cloudposse/terraform-aws-dynamodb/releases

12:53:56 AM

I’m unable to merge the pr tho :(

12:54:33 AM

May i have merge permissions?

If not, could you merge for me?

Dan Miller (Cloud Posse)

12:56:17 AM

It could be that it was out of date with main. Can you check again once the checks pass? And if you can’t merge then I will. But it would be helpful to know if others cant merge on their own

Dan Miller (Cloud Posse)

12:56:48 AM

/usr/lib/bats-core/test_functions.bash: line 161: terraform: command not found

Hmm that’s a different error. Will raise it internally

03:05:02 AM

It could be that it was out of date with main. when I got approval, it was in line with main and I still could not merge

03:05:11 AM

Will raise it internally thanks!

Dan Miller (Cloud Posse)

03:06:12 AM

Oh that’s good to know, thanks. Then I’ll merge once we get bats resolved

Dan Miller (Cloud Posse)

06:03:33 PM

fixed and merged

2024-05-23

2024-05-24

2024-05-25

Erik Osterman (Cloud Posse)

02:47:02 PM

https://sweetops.slack.com/archives/CB6GHNLG0/p1716562379529749

Not sure what your preferred workflow is, but maybe worth sharing here. I believe since the latest aws provider was released the route53 alias module is no longer useable. Several teams internally have reported the same issue I had, so I’d be surprised if any implementations are not suffering the same. I opened #53 therefore.

2024-05-27

Eligio Mariño

08:44:13 PM

Hi. Could you please review this MR in the AWS DocumentDB module? https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/94

#94 feat(aws_docdb_cluster): add allow_major_version_upgrade argument

what

This PR adds the argument allow_major_version_upgrade that was released in https://github.com/hashicorp/terraform-provider-aws/releases/tag/v5.21.0

When testing, I noticed that -get-plugins is not supporting anymore, as mentioned in Terraform documentation. So I removed TF_CLI_ARGS_init in the test Makefile.

test/src/go.mod was updated as a result of setting up to run the tests in test/src.

why

When upgrading the engine_version to a new major version, allow_major_version_upgrade needs to be enabled for AWS to apply the upgrade.

references

• https://github.com/hashicorp/terraform-provider-aws/releases/tag/v5.21.0 • https://awscli.amazonaws.com/v2/documentation/api/latest/reference/docdb/modify-db-cluster.html • https://docs.aws.amazon.com/documentdb/latest/developerguide/docdb-mvu.html

Marat Bakeev

05:09:38 AM

Please have a look at this trivial PR, that would fix issue in the terraform-aws-components https://github.com/cloudposse/terraform-aws-components/pull/1048

#1048 [vpc] update dynamic-subnet module version

what

Update dynamic-subnets module version to fix issue #1047

why

Version 2.3.0 of dynamic-subnets does not support the new opt-in Melbourne region.

references

This change in utils module adds support for Melbourne (amongst other regions) - cloudposse/terraform-aws-utils#26
And this is the version of dynamic-subnets where it was updated to use the updated utils module. https://github.com/cloudposse/terraform-aws-dynamic-subnets/releases/tag/2.4.0

This PR closes #1047

Marat Bakeev

05:10:10 AM

https://sweetops.slack.com/archives/CDYGZCLDQ/p1716782475227659 this is the original issue.

Hi guys, there seems to be an issue with the VPC component - would it be possible to update the version of dynamic-subnet within it, so we can use ap-southeast-4 Melbourne? Details are here - https://github.com/cloudposse/terraform-aws-components/issues/1047

2024-05-29

Quentin BERTRAND

09:31:59 AM

Hello, Small PR to add missing AMI type in EKS Node group module https://github.com/cloudposse/terraform-aws-eks-node-group/pull/182

#182 Chore: add missing AMI types

what

Adding missing AMI types

why

AL2023_x86_64_STANDARDand AL2023_ARM_64_STANDARD are available.

references

https://docs.aws.amazon.com/eks/latest/APIReference/API_Nodegroup.html#AmazonEKS-Type-Nodegroup-amiType

Quentin BERTRAND

09:08:28 AM

Hello there

#182 Chore: add missing AMI types

what

Adding missing AMI types

why

AL2023_x86_64_STANDARDand AL2023_ARM_64_STANDARD are available.

references

https://docs.aws.amazon.com/eks/latest/APIReference/API_Nodegroup.html#AmazonEKS-Type-Nodegroup-amiType

Erik Osterman (Cloud Posse)

05:48:42 PM

@Gabriela Campana (Cloud Posse) please help follow up

Gabriela Campana (Cloud Posse)

08:05:49 PM

This has been closed See why here: https://github.com/cloudposse/terraform-aws-eks-node-group/pull/182#issuecomment-2148313850

Comment on #182 Chore: add missing AMI types

@QuentinBtd Thank you for this PR. Unfortunately, it is not as simple as that.

See:

• #178 (first attempt to add AL2023 support) • #180

I am closing this PR. You may want to consider contributing to #180, or wait until we get around to adding this support (not currently scheduled).

2024-05-31

Eligio Mariño

06:16:58 PM

Hi. Is it possible to continue the review? The terratest command and the rest of checks have passed. https://sweetops.slack.com/archives/CUJPCP1K6/p1716842653008469

Hi. Could you please review this MR in the AWS DocumentDB module? https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/94

Erik Osterman (Cloud Posse)

06:54:38 PM

@Gabriela Campana (Cloud Posse)

Hi. Could you please review this MR in the AWS DocumentDB module? https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/94

Erik Osterman (Cloud Posse)

05:49:57 PM

@Gabriela Campana (Cloud Posse) can you help follow up with @Igor Rodionov who is working on it

Gabriela Campana (Cloud Posse)

08:41:18 PM

Asked @Igor Rodionov @Jeremy G (Cloud Posse) about the next steps

Gabriela Campana (Cloud Posse)

10:22:15 PM

Jeremy added the next steps to move this forward in his latest comment

Comment on #94 feat(aws_docdb_cluster): add allow_major_version_upgrade argument

@gmeligio Thank you for the offer to update the tests. It’s a big job and one that will take us a while to review, so please only do it if you are excited about the task. Here are my notes on what I would do, but they are shorthand and more needs to be done.

• test/Makefile and test/src/Makefile need to be updated. • go.mod is at go version 1.14, should be updated to 1.21 and dependencies updated. • Test should be updated to use CopyTerraformFolderToTemp to allow for real parallel testing • Test for enabled=false should be added • strconv.Itoa(rand.Intn(100000)) should be replaced with strings.ToLower(random.UniqueId()) and randId replaced with randID

The general pattern of starting and cleaning up tests has changed, with the above noting some of the more detailed changes.

• Dynamic subnets is a good, though perhaps overly complicated, example. • You can look at EKS workers, too, but it is not fully up to par. • Terraform example module gives you the basic idea of where we were a few years ago, and is cleaner and easier to understand.

If you are really ambitious, you can try reorganizing the code to accomplish the following objectives:

• Move more boilerplate/support to a separate file that can be common to all our tests. Things like set up and cleanup and random ID and error handling and Kubernetes and AWS clients could all go in there. The idea being that we should be able to update that file to be the same in every repo as our test framework evolves, leaving the test function focused on providing inputs, validating outputs, and making any other checks they need to make. • Add an option to run the tests only as far as the plan phase and check the results. This would give us an option to test multiple versions of Terraform and OpenTofu for syntax and features without requiring us to go through all of the resource creation and cleanup each time.

I know the above is a lot to ask, so please do not feel any obligation to do it.

Eligio Mariño

06:05:58 PM

Thank you for the update. I just replied in the issue and I’ll follow those instructions.

Jeremy G (Cloud Posse)

10:36:33 PM

Thank you, @Eligio Mariño. In order to make the PR easier to review, please keep commits making changes to the test framework separate from commits making changes to the module. Then we can review the PR one commit at at time if we want to simplify things without it getting too confusing.

Eligio Mariño

04:55:06 PM

Sounds good. I’ll do that.

Eligio Mariño

10:34:46 AM

I’m actually thinking about creating a new PR only for the changes in the test framework. It will be easier for me to follow if I separate those changes in the review discussion from the change I actually wanted to make the first time. Is it OK if I do it like that?

Erik Osterman (Cloud Posse)

01:06:29 PM

@Jeremy G (Cloud Posse)

Jeremy G (Cloud Posse)

12:24:05 AM

@Eligio Mariño If you want to update the test framework in a separate PR, that would be easier for us, too. Just know that the test PR will have to be approved and merged before your substantive PR 94 can be.

Jeremy G (Cloud Posse)

10:57:50 PM

@Eligio Mariño You can take a look at this commit to see an example of splitting up the test framework into multiple files.

Eligio Mariño

01:01:19 AM

Thank you @Jeremy G (Cloud Posse). I’ll look into that commit.

Eligio Mariño

12:05:00 AM

Hi @Jeremy G (Cloud Posse) . I created a new thread here just for the test framework PR. https://sweetops.slack.com/archives/CUJPCP1K6/p1718841858281129

Hi. Could you please review this PR to update the test framework in terraform-aws-documentdb-cluster. https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/100

Eligio Mariño

10:35:35 AM

Hi @Jeremy G (Cloud Posse). I updated my branch for this PR and re-requested a review. Just wanted to remind you here, too. I hope I’m not making too much noise.

Thank you in advance for taking this into consideration. https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/94

#94 feat(aws_docdb_cluster): add allow_major_version_upgrade argument

what

This PR adds the argument allow_major_version_upgrade that was released in https://github.com/hashicorp/terraform-provider-aws/releases/tag/v5.21.0

It includes with the changes in the test framework from #100 .

why

When upgrading the engine_version to a new major version, allow_major_version_upgrade needs to be enabled for AWS to apply the upgrade.

references

Jeremy G (Cloud Posse)

07:58:12 AM

@Eligio Mariño No problem reminding me/us here, that is what this Slack Channel is for!

You PR has been approved and merged. Thank you for all your extra work for such a simple change, and for your patience.

Eligio Mariño

11:47:36 AM

Thank you @Jeremy G (Cloud Posse). It was worth the wait. Now I know my change was better tested. Thank you for all your guidance and reviews.