#pr-reviews (2021-05)

Pull Request Reviews for Cloud Posse Projects

2021-05-31

2021-05-26

Frank avatar
Frank
feat: add exec support to task by syphernl · Pull Request #148 · cloudposse/terraform-aws-ecs-web-app attachment image

what Pass along the exec_enabled variable into ecs-alb-service-task why To be able to use exec on ECS tasks references cloudposse/terraform-aws-ecs-alb-service-task#113

1
1

2021-05-23

Matteo avatar
Matteo
Setting ca_cert_identifier default value to null by nnsense · Pull Request #115 · cloudposse/terraform-aws-rds attachment image

what Setting ca_cert_identifier default value to null, leaving its resolution up to the AWS resournce why Creating RDS in newer regions such as eu-south-1, ap-east-1, me-south-1, af-south-1 errors …

1

2021-05-20

Kevin Mahoney avatar
Kevin Mahoney

Hi

Please give a super quick review to This PR

2021-05-19

2021-05-14

2021-05-07

2021-05-06

2021-05-05

Alex Taylor avatar
Alex Taylor

https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/116 We’ve created a draft based on the feedback from above ^^

This has added a third ecs service that right now only ignores the changes to desired_count (So we don’t have one for both desired count + task definition because right now we don’t have the use-case for it, I’d rather that be added later if its needed) - want to know any thoughts on this before creating a PR and changing our code / moving remote state to avoid any unnecessary changes

Add third ECS Service with ignore desired_count by realrill · Pull Request #116 · cloudposse/terraform-aws-ecs-alb-service-task attachment image

what Add ignore_desired_count aws_ecs_service that has lifecycle setup to ignore desired_count changes why Desired count is a rough estimate that can be volatile during peak hours of the service…

Alex Taylor avatar
Alex Taylor

This is ready for a re-review now

Add third ECS Service with ignore desired_count by realrill · Pull Request #116 · cloudposse/terraform-aws-ecs-alb-service-task attachment image

what Add ignore_desired_count aws_ecs_service that has lifecycle setup to ignore desired_count changes why Desired count is a rough estimate that can be volatile during peak hours of the service…

Alex Taylor avatar
Alex Taylor

@RB (Ronak) (Cloud Posse) is there any chance I could get this reviewed please?

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

i ran the tests but the tests failed. please take a look when time permits

Alex Taylor avatar
Alex Taylor

@RB (Ronak) (Cloud Posse) Please can you take another look, cheers

Alex Taylor avatar
Alex Taylor

Any chance we can get a stamp of approval on this one?

Alex Taylor avatar
Alex Taylor

The tests all passed on this one, can we get an approval please?

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

It hurts to approve that one but no one can think of an alternative other than using custom terraform. The module now has 4 ecs service resources with copied and pasted terraform resource inputs

Alex Taylor avatar
Alex Taylor

Yeah until terraform make the lifecycle of a resource configurable at the module level I don’t think theres much we can do. Right now this change is breaking for us anyway (We need to terraform state mv from the old location to the new). If the underlying terraform resource for the ECS service ignored the value of desired count if its null that would have avoided this whole issue but what can you do

2021-05-04

Alex Taylor avatar
Alex Taylor
Add desired_count to ignore_changes by realrill · Pull Request #115 · cloudposse/terraform-aws-ecs-alb-service-task attachment image

what Ignoring desired count changes for aws_ecs_service why Desired count is a rough estimate that can be volatile during peak hours of the service that the containers serve Autoscaling policies…

1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

I don’t want to ignore the desired count in my ecs service tho

Add desired_count to ignore_changes by realrill · Pull Request #115 · cloudposse/terraform-aws-ecs-alb-service-task attachment image

what Ignoring desired count changes for aws_ecs_service why Desired count is a rough estimate that can be volatile during peak hours of the service that the containers serve Autoscaling policies…

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

and the lifecycle meta argument does not allow variable interpretation

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

the only way you could do it, is if you added a new aws_ecs_service that could be toggled but since youd also have to account for ignore the task definition or not ignoring the task definition, you have to add 2 new aws_ecs_service resources…….

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

it would be easier to create a toggle to disable the creation of the ecs service inside the tf module and simply create it outside of the module definition and then you can ignore changes on any resource argument you like

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

but… at that point, you might as well just use custom tf code… oof this is where terraform kind of stinks

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

if you set the var.desired_count = null, does it ignore the input auto-magically ?

Alex Taylor avatar
Alex Taylor

:thinking_face: I see. Yeah I understand that there are use-cases when you’d want to set the desired count every time.

So desired count defaults to 0 … And you specify the min & max through aws_appautoscaling_target resource…

Alex Taylor avatar
Alex Taylor

Let me see if I set that back to default/null what happens

Alex Taylor avatar
Alex Taylor

I have a feeling this isn’t going to work though, as the docs specifically point out ignoring desired_count https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ecs_service#ignoring-changes-to-desired-count

But worth testing it nonetheless!

Alex Taylor avatar
Alex Taylor

Nope I set it to null and its set desired_count to 0! Not one for production then

1
Alex Taylor avatar
Alex Taylor

Ok we’ve closed the PR. Will have to write some custom TF code then I guess

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

It’s a difficult issue to solve but worth discussing. The method of creating 2 new ecs service resources with the desired permutations of (3) ignoring desired count and (4) ignoring desired count and task def (where (1) is ignoring nothing and (2) is ignoring the task def) might be the only option here

Alex Taylor avatar
Alex Taylor

Agreed. It doesn’t look like terraform will allow interpolations in the lifecycle block anytime soon (I can only assume this is a complex thing to do). Looks like the issue was first raised in 2015

I wouldn’t want to end up in a scenario where we have loads of ECS services declared though for every possible combination, that would be a maintenance nightmare. Seems like being able to specify lifecycle attributes at the module level would be best (https://github.com/hashicorp/terraform/issues/27360)

Add support for lifecycle meta-argument in modules · Issue #27360 · hashicorp/terraform attachment image

Current Terraform Version Terraform v0.14.3 Use-cases Terraform currently only allows the lifecycle meta-argument to be used within the declaration of a resource. It would be really useful if users…

1
Alex Taylor avatar
Alex Taylor

I’ll go with creating the 4 different permutations for now and hope that this would be better solved in the future

    keyboard_arrow_up