feat: Only tag ecs service if longer arns are enabled in the aws account #153
Conversation
|
@antonbabenko ping |
Co-authored-by: Flaudísio Tolentino <[email protected]>
|
FYI, just a nit. if tags are changed out of state this will also cause a problem when it tries to remove tags from an ecs resource. |
True but the whole point of terraform is to do everything in code. Anything outside of code is a workaround. |
main.tf
Outdated
| @@ -608,7 +608,7 @@ resource "aws_ecs_service" "atlantis" { | |||
| } | |||
| } | |||
|
|
|||
| tags = local.tags | |||
| tags = var.use_old_arn_format ? null : local.tags | |||
There was a problem hiding this comment.
I don't have any way of testing since I don't have any accounts that do not use the newer format, but what if we do this instead (related to this comment)
| tags = var.use_old_arn_format ? null : local.tags | |
| tags = var.use_old_arn_format ? {} : local.tags |
There was a problem hiding this comment.
It's always better to do null since that safely removes the argument from being set in the first place.
We've been using this method in the https://github.com/cloudposse/terraform-aws-ecs-alb-service-task module with positive results.
There was a problem hiding this comment.
I don't believe it always removes, which is why I was suggesting an empty map which removes any ambiguity. I do agree that null works great as a default variable value, but this is something slightly different - a different edge case. either way, we can try this approach. one thing though - I think we should adjust the variable name to use_ecs_old_arn_format to make it a bit more clear
There was a problem hiding this comment.
I don't believe it always removes
I tested this with 0.12.x, 0.13.x, and 0.14.x and it seems to remove the argument completely from the resource.
either way, we can try this approach.
Ok 😄
one thing though - I think we should adjust the variable name to use_ecs_old_arn_format to make it a bit more clear
Agreed, merge conflicts fixed and that change was committed.
|
@nitrocode can you rebase? otherwise LGTM @antonbabenko |
|
Thanks! v2.41.0 has been just released. |
|
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. |
Description
Closes #152
Motivation and Context
Older AWS accounts that have not enabled the longer arn format will not be able to tag the ecs service. This will allow disabling tagging of the ecs service using a boolean.
Breaking Changes
None
How Has This Been Tested?
Locally