#pr-reviews (2021-01)
Pull Request Reviews for Cloud Posse Projects
2021-01-02
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
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.
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.
TF module will create the roles but if I try to pass roles in I got some errors as above
Are you able to share your module code? Maybe just the parts that are failing.
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
}
I used count
to create multiple services
the module creates roles if no roles are passed
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.
I’m not sure about solving the error above. It doesn’t make sense to me without running it.
thanks Joe, I am looking into the codes and see if it is possible to use count
I can call the module twice for 2 services as a workaround
Sorry, also meant to add that you should use for_each instead of count, in my opinion
got it
do we need list
type for all 3 roles, service
/execute
/task
?
2021-01-03
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
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
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
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
yea my bad
im testing now with process_domain_validation_options set to false
ok
ok all set
just tested with every option of process_domain_validation_options
and wait_for_certificate_issued
ok, cool
2021-01-06
2021-01-07
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.
@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.
@Alex Taylor will check for it first thing my morning. Thanks for letting us know about the problem
No problem thank you
2021-01-08
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
@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
fyi…
@Erik Osterman (Cloud Posse) @Matt Gowie @jose.amengual pr open… https://github.com/cloudposse/build-harness/pull/272
Will review on Monday
@Erik Osterman (Cloud Posse) @Matt Gowie @jose.amengual pr open… https://github.com/cloudposse/build-harness/pull/272
@Jeremy G (Cloud Posse)
2021-01-11
2021-01-26
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…