#pr-reviews (2021-01)

Pull Request Reviews for Cloud Posse Projects

2021-01-11

2021-01-09

loren avatar
loren
03:58:21 PM

fyi…

@Erik Osterman (Cloud Posse) @Matt Gowie @PePe pr open… https://github.com/cloudposse/build-harness/pull/272

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

Will review on Monday

@Erik Osterman (Cloud Posse) @Matt Gowie @PePe pr open… https://github.com/cloudposse/build-harness/pull/272

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

@Jeremy (Cloud Posse)

2021-01-08

Mikhail Naletov avatar
Mikhail Naletov
Add customisable aws log prefix by okgolove · Pull Request #79 · cloudposse/terraform-aws-ecs-web-app

what Customisable aws log prefix why May be helpful in case of you don’t use name in label module

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

@Maxim Mironenko (Cloud Posse)

Add customisable aws log prefix by okgolove · Pull Request #79 · cloudposse/terraform-aws-ecs-web-app

what Customisable aws log prefix why May be helpful in case of you don’t use name in label module

2021-01-07

Alex Taylor avatar
Alex Taylor

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

Add scram authentication by taylora433 · Pull Request #6 · cloudposse/terraform-aws-msk-apache-kafka-cluster

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.

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

@Maxim Mironenko (Cloud Posse)

Add scram authentication by taylora433 · Pull Request #6 · cloudposse/terraform-aws-msk-apache-kafka-cluster

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
Maxim Mironenko (Cloud Posse)

@ will check for it first thing my morning. Thanks for letting us know about the problem

Alex Taylor avatar
Alex Taylor

No problem thank you

2021-01-06

2021-01-04

pjaudiomv avatar
pjaudiomv
remove uneeded tolist as is set now by pjaudiomv · Pull Request #38 · cloudposse/terraform-aws-acm-request-certificate

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

1
PePe avatar

is it ok to merge? any more changes?

remove uneeded tolist as is set now by pjaudiomv · Pull Request #38 · cloudposse/terraform-aws-acm-request-certificate

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
pjaudiomv

yea my bad

pjaudiomv avatar
pjaudiomv

im testing now with process_domain_validation_options set to false

PePe avatar

ok

pjaudiomv avatar
pjaudiomv

ok all set

pjaudiomv avatar
pjaudiomv

just tested with every option of process_domain_validation_options and wait_for_certificate_issued

PePe avatar

ok, cool

2021-01-03

pjaudiomv avatar
pjaudiomv
Terraform 0.14 upgrade by pjaudiomv · Pull Request #39 · cloudposse/terraform-aws-route53-cluster-zone

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

1
1
pjaudiomv avatar
pjaudiomv
updates for tf 0.14 and aws provder 3.x by pjaudiomv · Pull Request #35 · cloudposse/terraform-aws-acm-request-certificate

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

1
1

2021-01-02

Hao Wang avatar
Hao Wang
Joe Niland avatar
Joe Niland
Task definition parameters - Amazon Elastic Container Service

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
Hao Wang

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
Hao Wang

TF module will create the roles but if I try to pass roles in I got some errors as above

Joe Niland avatar
Joe Niland

Are you able to share your module code? Maybe just the parts that are failing.

Hao Wang avatar
Hao Wang
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
Hao Wang

I used count to create multiple services

Hao Wang avatar
Hao Wang

the module creates roles if no roles are passed

Joe Niland avatar
Joe Niland

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
Joe Niland

I’m not sure about solving the error above. It doesn’t make sense to me without running it.

Hao Wang avatar
Hao Wang

thanks Joe, I am looking into the codes and see if it is possible to use count

Hao Wang avatar
Hao Wang

I can call the module twice for 2 services as a workaround

Joe Niland avatar
Joe Niland

Sorry, also meant to add that you should use for_each instead of count, in my opinion

Hao Wang avatar
Hao Wang

got it

Hao Wang avatar
Hao Wang

do we need list type for all 3 roles, service/execute/task?

    keyboard_arrow_up