#pr-reviews (2022-10)

Pull Request Reviews for Cloud Posse Projects

2022-10-04

mihai.plesa avatar
mihai.plesa

what

• The current input variable service_registries requires all params to be defined. • This is rejected by AWS API, so currently it is not useable! • This PR allows any combination of input params for service_registries

why

• Currently if you do not use all input params, terraform would reject the code with errors attributes "container_name" and "container_port" are required • And if you try to supply all params, AWS API validation fails with Error: failed creating ECS service (EXAMPLE): InvalidParameterException: Specify a value for either 'port' or the 'containerName' and 'containerPort' combination, but not both. Remove one and retry. • This PR copies the variable definition from the upstream module terraform-aws-ecs-alb-service-task (code). This allows multiple possible combinations of input params as it should

Joe Niland avatar
Joe Niland

Hey @mihai.plesa we were discussing whether to implement optional attributes and thus only support TF 1.3+

In my own use of this module, I found that it will work if the port values are set to 0, although I can’t comment on all possible cases.

what

• The current input variable service_registries requires all params to be defined. • This is rejected by AWS API, so currently it is not useable! • This PR allows any combination of input params for service_registries

why

• Currently if you do not use all input params, terraform would reject the code with errors attributes "container_name" and "container_port" are required • And if you try to supply all params, AWS API validation fails with Error: failed creating ECS service (EXAMPLE): InvalidParameterException: Specify a value for either 'port' or the 'containerName' and 'containerPort' combination, but not both. Remove one and retry. • This PR copies the variable definition from the upstream module terraform-aws-ecs-alb-service-task (code). This allows multiple possible combinations of input params as it should

1
mihai.plesa avatar
mihai.plesa

cc @Ahmed Kamal

1

2022-10-06

2022-10-07

Quentin BERTRAND avatar
Quentin BERTRAND

Hello I opened this PR to add the option table_class to the DynamoDB module https://github.com/cloudposse/terraform-aws-dynamodb/pull/106

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

@Andriy Knysh (Cloud Posse)

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

approved , thanks @Quentin BERTRAND

Quentin BERTRAND avatar
Quentin BERTRAND

Thank you too !

2022-10-10

2022-10-11

2022-10-14

Joe Niland avatar
Joe Niland

what

• Adds a random string to the codepipeline module’s attributes to provide a unique name for the codepipeline bucket

why

• Automated tests sometimes fail due to bucket naming collisions (presumably when a previous test run failed and was not properly cleaned up)

references

• None

1
1

2022-10-19

Quentin BERTRAND avatar
Quentin BERTRAND

what

Use latest versions of the s3-log-storage module and label null

why

Be up to date

references

https://github.com/cloudposse/terraform-aws-s3-log-storage/releases/tag/0.28.3
https://github.com/cloudposse/terraform-null-label/releases

mihai.plesa avatar
mihai.plesa

PR to not create CodePipeline resources when it is disabled https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/210

Resolves #211

mihai.plesa avatar
mihai.plesa

hey @RB, are you happy with the PR after all?

Resolves #211

RB avatar

I added some other people on the PR to review.

mihai.plesa avatar
mihai.plesa

okay

mihai.plesa avatar
mihai.plesa

rebased and could use another test run

2022-10-20

Karina Titov avatar
Karina Titov

Hi! PR to add an ability to set the custom iam policy name https://github.com/cloudposse/terraform-aws-iam-role/pull/50

have a chance to configure the name of the policy

what

• With this change i want to have an ability to provide a custom name for the policy

why

• the resources i’m working with were not created in the same way this module assumes • to have a chance to configure the name of the policy

2022-10-21

Karina Titov avatar
Karina Titov

Hi everyone! https://github.com/cloudposse/terraform-aws-iam-role/pull/50 i got 1 approval on my pr, but still can not merge, could somebody please help me merge

have a chance to configure the name of the policy

what

• With this change i want to have an ability to provide a custom name for the policy

why

• the resources i’m working with were not created in the same way this module assumes • to have a chance to configure the name of the policy

Karina Titov avatar
Karina Titov

@Cody Moore

2022-10-24

comrumino avatar
comrumino

Hi all, I made two PRs that relatively small changes. Lmk if I need to fix/change anything. #215 resource specification would be the part most worth reviewing. #214 is a simple argument update to stop using deprecated one

https://github.com/cloudposse/terraform-aws-elastic-beanstalk-environment/pull/214

https://github.com/cloudposse/terraform-aws-elastic-beanstalk-environment/pull/215

1
comrumino avatar
comrumino

@cloudposse-team @Andriy Knysh (Cloud Posse)

comrumino avatar
comrumino

I amended them to fix commit auth for gpg signature to show as verified

2022-10-25

Colin Horgan avatar
Colin Horgan

what

• This PR will introduce logic to ignore certain CIDR ranges from either the requestor or acceptor VPC(s), without affecting the existing functionality.

why

• We add the CIDR range 100.64.0.0/16 to each of our EKS cluster VPCs to increase the maximum amount of assignable IP addresses when using the AWS VPC CNI. • This causes issues when attempting to leverage this existing peering module, as the 100.64.0.0/16 routes will conflict in certain cases. • One specific case is where we want to peer and route a single database or elasticsearch VPC to multiple different EKS cluster VPCs that all contain the 100.64.0.0/16 additional CIDR association

references

https://aws.amazon.com/premiumsupport/knowledge-center/eks-multiple-cidr-ranges/

2022-10-26

mihai.plesa avatar
mihai.plesa

seeing an older one here that passed tests. can it go in? https://github.com/cloudposse/terraform-aws-alb-ingress/pull/57

2022-10-27

Nitin avatar

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.

why

• Provide the justifications for the changes (e.g. business case). • Describe why these changes were made (e.g. why do these commits fix the problem?) • Use bullet points to be concise and to the point.

references

• Link to any supporting github issues or helpful documentation to add some context (e.g. stackoverflow). • Use closes #123, if this PR closes a GitHub issue #123

Nitin avatar

what

• IPv6 related descritption typo fix

why

• In IPv6 related outputs vars descrption contains IPv4 instead of IPv6

2022-10-28

Nitin avatar

what

• This PR will introduce logic to ignore certain CIDR ranges from either the requestor or acceptor VPC(s), without affecting the existing functionality.

why

• We add the CIDR range 100.64.0.0/16 to each of our EKS cluster VPCs to increase the maximum amount of assignable IP addresses when using the AWS VPC CNI. • This causes issues when attempting to leverage this existing peering module, as the 100.64.0.0/16 routes will conflict in certain cases. • One specific case is where we want to peer and route a single database or elasticsearch VPC to multiple different EKS cluster VPCs that all contain the 100.64.0.0/16 additional CIDR association

references

https://aws.amazon.com/premiumsupport/knowledge-center/eks-multiple-cidr-ranges/

Nitin avatar

what

• Use cloudposse/template for arm support

why

• The new cloudposse/template provider has a darwin arm binary for M1 laptops

references

https://github.com/cloudposse/terraform-provider-templatehttps://registry.terraform.io/providers/cloudposse/template/latest

1
1
RB avatar

cc @Andriy Knysh (Cloud Posse) please review

what

• Use cloudposse/template for arm support

why

• The new cloudposse/template provider has a darwin arm binary for M1 laptops

references

https://github.com/cloudposse/terraform-provider-templatehttps://registry.terraform.io/providers/cloudposse/template/latest

1
RB avatar

@Nitin i guess this pr didnt matter so much because it was a no release meaning the update was only for the examples and not the actual module. The module should work fine on an m1 before and after that pr merge

2022-10-29

Nitin avatar

what

• Allow IPv6 communication b/w VPCs over VPC peering

why

• Application hosted in VPC-1 wants to access resources hosted in VPC-2 in private subnet over IPv6.

    keyboard_arrow_up