#pr-reviews (2021-01)
Pull Request Reviews for Cloud Posse Projects
2021-01-02
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
Hi, I closed this PR for it may need more fixes, https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/89/commits/4c47d80f4d9aa51b23f51b3861b93a2008f9871c
![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)
Don’t those parameters take a single role? https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html
Task definitions are split into separate parts: the task family, the IAM task role, the network mode, container definitions, volumes, task placement constraints, and launch types. The family and container definitions are required in a task definition, while task role, network mode, volumes, task placement constraints, and launch type are optional.
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
got this error from TF module:
Error: Invalid count argument
on .terraform/modules/ecs_alb_service_tasks/main.tf line 99, in data "aws_iam_policy_document" "ecs_task":
99: count = local.enabled && length(var.task_role_arn) == 0 ? 1 : 0
The "count" value depends on resource attributes that cannot be determined
until apply, so Terraform cannot predict how many instances will be created.
To work around this, use the -target argument to first apply only the
resources that the count depends on.
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
TF module will create the roles but if I try to pass roles in I got some errors as above
![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)
Are you able to share your module code? Maybe just the parts that are failing.
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
module "ecs_alb_service_tasks" {
source = "cloudposse/ecs-alb-service-task/aws"
version = "0.42.3"
count = length(var.container_definitions)
attributes = [var.container_definitions[count.index].container_name]
alb_security_group = module.vpc.vpc_default_security_group_id
use_alb_security_group = true
#task_exec_role_arn = aws_iam_role.ecs_exec.arn
#task_role_arn = aws_iam_role.ecs_task.arn
container_definition_json = module.container_definitions[count.index].json_map_encoded_list
ecs_cluster_arn = aws_ecs_cluster.default.arn
launch_type = var.ecs_launch_type
vpc_id = module.vpc.vpc_id
security_group_ids = [module.vpc.vpc_default_security_group_id]
subnet_ids = module.subnets.public_subnet_ids
ignore_changes_task_definition = var.ignore_changes_task_definition
network_mode = "bridge"
assign_public_ip = var.assign_public_ip
propagate_tags = var.propagate_tags
deployment_minimum_healthy_percent = var.deployment_minimum_healthy_percent
deployment_maximum_percent = var.deployment_maximum_percent
deployment_controller_type = var.deployment_controller_type
desired_count = var.desired_count
task_memory = var.task_memory
task_cpu = var.task_cpu
}
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
I used count
to create multiple services
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
the module creates roles if no roles are passed
![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)
I think I would use a map variable with the key being the name of each image. Using count will make it hard for you to change things later.
![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)
I’m not sure about solving the error above. It doesn’t make sense to me without running it.
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
thanks Joe, I am looking into the codes and see if it is possible to use count
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
I can call the module twice for 2 services as a workaround
![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)
Sorry, also meant to add that you should use for_each instead of count, in my opinion
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
got it
![Hao Wang avatar](https://secure.gravatar.com/avatar/aa01de6ab42f1576bbb56a203c660939.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
do we need list
type for all 3 roles, service
/execute
/task
?
2021-01-03
![pjaudiomv avatar](https://secure.gravatar.com/avatar/40f13c8f113a13f5b9730c8cd47ec9ee.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
got a tf 0.14 upgrade PR here https://github.com/cloudposse/terraform-aws-route53-cluster-zone/pull/39
what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14 references trying to use this upstream with 0.14 and need to upgrade
![pjaudiomv avatar](https://secure.gravatar.com/avatar/40f13c8f113a13f5b9730c8cd47ec9ee.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
got another 0.14 PR https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/35
what Upgrade to support Terraform 0.14 and bring up to current Cloud Posse standard why Support Terraform 0.14 Support AWS Provider >= 3.x references https://registry.terraform.io/providers…
2021-01-04
![pjaudiomv avatar](https://secure.gravatar.com/avatar/40f13c8f113a13f5b9730c8cd47ec9ee.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
got a PR to fix a regression from my last PR https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/38
what there is no need to convert to a list anymore as its a set why fixes bug introduced by me :) references issue from slack thread https://sweetops.slack.com/archives/CB6GHNLG0/p1609784495001600
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
is it ok to merge? any more changes?
what there is no need to convert to a list anymore as its a set why fixes bug introduced by me :) references issue from slack thread https://sweetops.slack.com/archives/CB6GHNLG0/p1609784495001600
![pjaudiomv avatar](https://secure.gravatar.com/avatar/40f13c8f113a13f5b9730c8cd47ec9ee.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
yea my bad
![pjaudiomv avatar](https://secure.gravatar.com/avatar/40f13c8f113a13f5b9730c8cd47ec9ee.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
im testing now with process_domain_validation_options set to false
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
ok
![pjaudiomv avatar](https://secure.gravatar.com/avatar/40f13c8f113a13f5b9730c8cd47ec9ee.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
ok all set
![pjaudiomv avatar](https://secure.gravatar.com/avatar/40f13c8f113a13f5b9730c8cd47ec9ee.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0013-72.png)
just tested with every option of process_domain_validation_options
and wait_for_certificate_issued
![jose.amengual avatar](https://secure.gravatar.com/avatar/32f267b819eac9e0ea6a8324b53064a0.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0024-72.png)
ok, cool
2021-01-06
2021-01-07
![Alex Taylor avatar](https://secure.gravatar.com/avatar/0aec56e35622e38122cb4dd51086a0eb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0019-72.png)
Hello. Is anyone able to help me push through this PR? (I’m currently having to reference my fork in our code which is less than ideal) - It needs approval and unsure as to why the tests are failing as I haven’t changed the default behaviour. Thanks
https://github.com/cloudposse/terraform-aws-msk-apache-kafka-cluster/pull/6
what Allow cluster to be created with SCRAM authentication. why TLS authentication is not suitable in our use case, we need to use the SCRAM authentication mechanism.
![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)
@Maxim Mironenko (Cloud Posse)
what Allow cluster to be created with SCRAM authentication. why TLS authentication is not suitable in our use case, we need to use the SCRAM authentication mechanism.
![Maxim Mironenko (Cloud Posse) avatar](https://avatars.slack-edge.com/2019-01-14/522357805937_0a3ceb59638dd335f2c8_72.jpg)
@Alex Taylor will check for it first thing my morning. Thanks for letting us know about the problem
![Alex Taylor avatar](https://secure.gravatar.com/avatar/0aec56e35622e38122cb4dd51086a0eb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0019-72.png)
No problem thank you
2021-01-08
![Mikhail Naletov avatar](https://avatars.slack-edge.com/2021-08-27/2436309147940_5870ad432ab4889ee4a9_72.png)
Hello! Could somebody take a look? https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/79
what Customisable aws log prefix why May be helpful in case of you don’t use name in label module
![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)
@Maxim Mironenko (Cloud Posse)
what Customisable aws log prefix why May be helpful in case of you don’t use name in label module
2021-01-09
![loren avatar](https://secure.gravatar.com/avatar/d1e25dcfbc68a0857a04dd78c9afe952.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
fyi…
@Erik Osterman (Cloud Posse) @Matt Gowie @jose.amengual pr open… https://github.com/cloudposse/build-harness/pull/272
![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)
Will review on Monday
@Erik Osterman (Cloud Posse) @Matt Gowie @jose.amengual pr open… https://github.com/cloudposse/build-harness/pull/272
![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)
@Jeremy G (Cloud Posse)
2021-01-11
2021-01-26
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Hi there. Can someone please take a look at https://github.com/cloudposse/terraform-aws-dynamic-subnets/pull/115? The current version (with the default settings) is broken
what Add required http_endpoint and http_put_response_hop_limit parameters to the metadata_options section why Changes done in 886a164 results in a Terraform error while provisioning (A value of…