#pr-reviews (2022-10)
Pull Request Reviews for Cloud Posse Projects
2022-10-04
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
who can help with this quick one please? https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/208
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](https://secure.gravatar.com/avatar/b90c8e752dd648ef229096c60ba2408f.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0022-72.png)
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
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
2022-10-06
2022-10-07
![Quentin BERTRAND avatar](https://avatars.slack-edge.com/2022-08-29/4028696878592_774493d7ba3d4c45009e_72.jpg)
Hello
I opened this PR to add the option table_class
to the DynamoDB module
https://github.com/cloudposse/terraform-aws-dynamodb/pull/106
what
Adding table_class
option
why
This option can be usefull for tables with infrequent access. It can reduce costs by up to 60%.
references
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/dynamodb_table#table_class
https://aws.amazon.com/fr/dynamodb/standard-ia/
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Andriy Knysh (Cloud Posse)
what
Adding table_class
option
why
This option can be usefull for tables with infrequent access. It can reduce costs by up to 60%.
references
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/dynamodb_table#table_class
https://aws.amazon.com/fr/dynamodb/standard-ia/
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
approved , thanks @Quentin BERTRAND
![Quentin BERTRAND avatar](https://avatars.slack-edge.com/2022-08-29/4028696878592_774493d7ba3d4c45009e_72.jpg)
Thank you too !
2022-10-10
2022-10-11
2022-10-14
![Joe Niland avatar](https://secure.gravatar.com/avatar/b90c8e752dd648ef229096c60ba2408f.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0022-72.png)
Could I please get a review on https://github.com/cloudposse/terraform-aws-ecs-codepipeline/pull/102 ?
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
2022-10-19
![Quentin BERTRAND avatar](https://avatars.slack-edge.com/2022-08-29/4028696878592_774493d7ba3d4c45009e_72.jpg)
Hello Another PR, could you review it ? https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/pull/66
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](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
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](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
I added some other people on the PR to review.
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
okay
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
rebased and could use another test run
2022-10-20
![Karina Titov avatar](https://avatars.slack-edge.com/2022-06-22/3704988878467_96a8f818f4a1ce6884d3_72.jpg)
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](https://avatars.slack-edge.com/2022-06-22/3704988878467_96a8f818f4a1ce6884d3_72.jpg)
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](https://avatars.slack-edge.com/2022-06-22/3704988878467_96a8f818f4a1ce6884d3_72.jpg)
@Cody Moore
2022-10-24
![comrumino avatar](https://secure.gravatar.com/avatar/a5221df725249e3a76fe7906d14502be.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0004-72.png)
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
![comrumino avatar](https://secure.gravatar.com/avatar/a5221df725249e3a76fe7906d14502be.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0004-72.png)
@cloudposse-team @Andriy Knysh (Cloud Posse)
![comrumino avatar](https://secure.gravatar.com/avatar/a5221df725249e3a76fe7906d14502be.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0004-72.png)
I amended them to fix commit auth for gpg signature to show as verified
2022-10-25
![Colin Horgan avatar](https://avatars.slack-edge.com/2022-10-25/4288014278017_6be532da3823f5490e20_72.png)
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](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
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](https://secure.gravatar.com/avatar/b5ebdeb86a36df979cbf426d73d1ff00.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
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](https://secure.gravatar.com/avatar/b5ebdeb86a36df979cbf426d73d1ff00.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
what
• IPv6 related descritption typo fix
why
• In IPv6 related outputs vars descrption contains IPv4 instead of IPv6
2022-10-28
![Nitin avatar](https://secure.gravatar.com/avatar/b5ebdeb86a36df979cbf426d73d1ff00.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
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](https://secure.gravatar.com/avatar/b5ebdeb86a36df979cbf426d73d1ff00.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
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-template • https://registry.terraform.io/providers/cloudposse/template/latest
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
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-template • https://registry.terraform.io/providers/cloudposse/template/latest
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
@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](https://secure.gravatar.com/avatar/b5ebdeb86a36df979cbf426d73d1ff00.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
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.