Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add option to ignore load_balancer changes to ECS service #81

Merged

Conversation

gpdenny
Copy link
Contributor

@gpdenny gpdenny commented May 2, 2023

Description

Create (another 😬) instance of aws_ecs_service to allow users to ignore load_balancer changes.

Motivation and Context

I understand this pattern could be never ending, and result in a module that's very difficult to maintain, but to me it feels like a fairly typical use-case to support Codedeploy with Blue/Green deployment.

I'm not sure if maybe it should be a single variable that controls this, instead of setting both ignore_task_definition_changes and ignore_load_balancer_changes to true?

Breaking Changes

No breaking changes, default behaviour is unchanged.

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
  • Tested all three scenarios where each of the instances of aws_ecs_service are used.
  • I have executed pre-commit run -a on my pull request

@gpdenny gpdenny changed the title feat: add option to ignore load_balancer changes to ECS service feat: Add option to ignore load_balancer changes to ECS service May 2, 2023
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Please update main.tf in the root module to include the same variables.

@@ -9,7 +9,7 @@ output "id" {

output "name" {
description = "Name of the service"
value = try(aws_ecs_service.this[0].name, aws_ecs_service.ignore_task_definition[0].name, null)
value = try(aws_ecs_service.this[0].name, aws_ecs_service.ignore_task_definition[0].name, aws_ecs_service.ignore_task_definition_load_balancer[0].name, null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A similar change should be implemented for output id.

docs/README.md Outdated
- When using the above `ignore_task_definition_changes` setting, users can also elect to ignore changes to load balancers by setting `ignore_load_balancer_changes` to `true`. (Note: because of the aforementioned manner in which this psuedo-dynamic ignore change is being managed, changing this value after service creation will cause the entire service to be re-created. Change with caution!) This is intended to support the use of [Blue/Green deployment with CodeDeploy](https://docs.aws.amazon.com/AmazonECS/latest/userguide/deployment-type-bluegreen.html) which changes the the service's load balancer configuration.

```hcl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@bryantbiggs
Copy link
Member

I think I am more inclined to add this to the ignore_task_definition version of the service rather than create yet another service definition. we can just add the load_balancer attribute to the ignore list here, update the docs, and then we can show in the docs a similar hack like that of updating the service desired count https://github.com/terraform-aws-modules/terraform-aws-ecs/blob/master/docs/README.md#service-1

So one service is static, all controlled in Terraform, the other one is dynamic for changing values

@gpdenny
Copy link
Contributor Author

gpdenny commented May 3, 2023

Agree @bryantbiggs this sounds less likely for things to get out of hand in future 😂
Should we rename the ignore_task_definition instance as well if we are adding more to the ignore list? Any preference?

@bryantbiggs
Copy link
Member

we don't need to rename it at this time since that will be a breaking change. Lets just ensure its properly documented

@david1437
Copy link

Hi, this feature is very important for my company's use case is there any way I can help to move this forward?

@gpdenny
Copy link
Contributor Author

gpdenny commented Jun 3, 2023

Hey @david1437 I've been distracted changing day jobs, but I'll still need this in my new role.
Hoping to tackle some point next week 👌

@bryantbiggs
Copy link
Member

@antonbabenko let me know what you think

docs/README.md Outdated Show resolved Hide resolved
@bryantbiggs bryantbiggs merged commit 24bd1d8 into terraform-aws-modules:master Jun 5, 2023
antonbabenko pushed a commit that referenced this pull request Jun 5, 2023
## [5.1.0](v5.0.2...v5.1.0) (2023-06-05)

### Features

* Add option to ignore `load_balancer` changes to ECS service ([#81](#81)) ([24bd1d8](24bd1d8))
@antonbabenko
Copy link
Member

This PR is included in version 5.1.0 🎉

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2023
@gpdenny gpdenny deleted the service-ignore-loadbalancer branch July 6, 2023 09:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants