feat!: Work in progress for v6

[bryantbiggs]

Description

Motivation and Context

• Resolves #147
• Resolves #151
• Resolves #164
• Resolves #165
• Resolves #221
• Resolves #235
• Resolves #240
• Resolves #241
• Resolves #258
• Resolves #262
• Resolves #265
• Resolves #281
• Resolves #289
• Closes #274

Breaking Changes

How Has This Been Tested?

• [ ] I have updated at least one of the examples/* to demonstrate and validate my change(s)
• [ ] I have tested and validated these changes using one or more of the provided examples/* projects

• [ ] I have executed pre-commit run -a on my pull request

[kkrastev-cloudoffice]

any ETA on this?

[BramRoets]

@bryantbiggs Could you please provide an update on this PR? It adds multiple important features which are standard to AWS ECS.

It's been open for a very long time. Is this still being maintained or should we move away from this project?

[bryantbiggs]

yes, part of it is finding time since these large, breaking changes do take a considerable amount of time to test and document, and part of it is balancing the number of times we take a breaking change (major version bump). with v6 of the provider coming, I'm half inclined to wait and set the minimum provider version to 6.0 https://github.com/hashicorp/terraform-provider-aws/issues/41101 to have a stable path forward for quite some time after

[nZac]

@bryantbiggs if I can be of use in testing I'd be glad to help run this through the paces. Do you have a pattern or practice you use and documentation you would need to validate that testing is sufficient?

[webyneter]

FYI, had to set these to work around validation errors, in a scenario when none of these were meant to be set :

tasks_iam_role_statements = []
security_group_ingress_rules = {}
security_group_egress_rules = {}

[webyneter]

Also, FYI, var.track_latest description is off: It says, the default is false whereas the actual default = true.

In addition to that, there's no way to set var.track_latest from the root module, there's neither a track_latest parameter on the module.service, nor the corresponding root module variable.

[nikita-toffee-ai]

The enabled field in the var.service_connect_configuration is now enabled = true by default, so even if I might not need service discovery, it now requires me to specify the namespace regardless.

[webyneter]

tag_specifications = optional(list(object({
        propagate_tags = optional(string, "TASK_DEFINITION")
        resource_type  = string
        tags           = optional(map(string))
      })))

I think, we should make resource_type = optional(string, "volume"), besides, "volume" is the only value that's currently allowed.

[webyneter]

The module/service's

variable "load_balancer" {
  description = "Configuration block for load balancers"
  type = map(object({
    container_name   = string
    container_port   = number
    elb_name         = optional(string)
    target_group_arn = optional(string)
  }))
  default = null
}

whereas the root service defaults to {}, contrary to the expected default = null. As a result, this no longer evaluates correctly:

# ...
locals {
  needs_iam_role  = var.network_mode != "awsvpc" && var.load_balancer != null
  # ...
}
# ...

[buffalmacco]

I tried the code in this branch with

create_task_definition = false

This is what I get:

│ Error: Invalid function argument
│
│   on .terraform/modules/ecs_service/modules/service/main.tf line 1166, in resource "aws_iam_role_policy" "tasks":
│ 1166:   count = local.create_tasks_iam_role && (length(var.tasks_iam_role_statements) > 0 || var.enable_execute_command) ? 1 : 0
│     ├────────────────
│     │ while calling length(value)
│     │ var.tasks_iam_role_statements is null
│
│ Invalid value for "value" parameter: argument must not be null.

(line 1166 because I was testing commit 75f3e7d310ba85e5ffbc249a7e9d150e1596086d)

The right side of the boolean operation is evaluated when the left side is false. As a fix I updated the line:

count = local.create_tasks_iam_role ? ((length(var.tasks_iam_role_statements) > 0 || var.enable_execute_command) ? 1 : 0) : 0

[bryantbiggs]

This PR is marked as a draft for a reason - I would not rely on it "working" while in draft mode

[bryantbiggs]

we'll proceed once AWS provider v6 arrives - this was originally meant to arrive in April but seems to be taking a bit longer

[zahorniak]

Hey @bryantbiggs, is there something I can help you with to make it work? I see a lot of open issues in the Milestone #1. Can I pick anything, or is there a priority for what should be fixed or implemented first?

[bryantbiggs]

note to self: default_capacity_provider_strategy was properly borked the way it was previously configured 😅

[willfish]

I'm integrating this in our repo, here, and all seems to work as before https://github.com/trade-tariff/trade-tariff-platform-aws-terraform/pull/416

[antonbabenko]

This PR is included in version 6.0.0 :tada:

[github-actions[bot]]

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.