Conversation
Pre-Commit report✅ All pre-commit checks are now passing! This comment will be updated when code changes. |
2e354aa to
d1a4917
Compare
There was a problem hiding this comment.
Pull request overview
This pull request refactors ECS scheduled tasks for forms-admin by creating a reusable ecs-scheduled-task module and a shared per-account IAM role for EventBridge. The refactor consolidates duplicate code from the mailchimp-sync and organisations-sync implementations into a generic module that can be used to define multiple scheduled tasks more easily.
Changes:
- Created a new
ecs-scheduled-taskmodule that encapsulates ECS task definition, EventBridge rule, and event target resources - Added a shared
ecsEventsRoleIAM role in the environment module for EventBridge to run ECS tasks - Refactored mailchimp-sync and organisations-sync to use the new module with a
for_eachloop inscheduled-tasks.tf
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| infra/modules/ecs-scheduled-task/main.tf | New generic module for creating ECS scheduled tasks with EventBridge triggers |
| infra/modules/ecs-scheduled-task/variables.tf | Input variables for the scheduled task module |
| infra/modules/ecs-scheduled-task/outputs.tf | Output for the EventBridge rule name |
| infra/modules/forms-admin/scheduled-tasks.tf | New file consolidating both scheduled tasks using the generic module with for_each |
| infra/modules/forms-admin/orgs-sync.tf | Removed task definition and EventBridge resources, kept failure monitoring |
| infra/modules/forms-admin/mailchimp-sync.tf | Removed task definition and EventBridge resources, kept failure monitoring |
| infra/modules/forms-admin/variables.tf | Added ecs_events_role_arn variable for the shared IAM role |
| infra/modules/environment/ecs.tf | Created shared ecsEventsRole IAM role for EventBridge |
| infra/modules/environment/outputs.tf | Added output for ecs_events_role_arn |
| infra/deployments/forms/forms-admin/main.tf | Passed ecs_events_role_arn to forms-admin module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for_each = local.scheduled_tasks | ||
| source = "../ecs-scheduled-task" | ||
|
|
||
| task_name = "forms-admin-${replace(each.key, "_", "-")}" |
There was a problem hiding this comment.
The task name has multiple critical issues:
-
Pipeline Breakage: The task family name has changed from
${var.environment_name}_forms-admin_mailchimp_sync(e.g.,production_forms-admin_mailchimp_sync) toforms-admin-mailchimp-sync. This breaks the deployment pipeline (deploy-forms-admin-container.tf lines 241 and 270) which expects the old naming pattern with underscores and environment prefix. The pipeline's update task definition steps will fail to find the correct tasks. -
Logging Issues: The task name is used as the CloudWatch log stream prefix (see ecs-scheduled-task/main.tf line 30). Without the environment name, logs from all environments will have the same prefix (
forms-admin-mailchimp-sync/main/*), making it impossible to filter by environment. The old code usedforms-admin-${var.env_name}-organisations-syncwhich included the environment. The troubleshooting documentation in orgs-sync.tf line 43 expects environment-specific log streams.
The task_name should be ${var.env_name}_forms-admin-${replace(each.key, "_", "-")} to match the old pattern and preserve both pipeline compatibility and environment-specific logging.
| task_name = "forms-admin-${replace(each.key, "_", "-")}" | |
| task_name = "${var.env_name}_forms-admin-${replace(each.key, "_", "-")}" |
| moved { | ||
| from = aws_ecs_task_definition.orgs_cron_job[0] | ||
| to = module.scheduled_tasks["organisations_sync"].aws_ecs_task_definition.this | ||
| } | ||
|
|
||
| moved { | ||
| from = aws_cloudwatch_event_rule.sync_orgs_cron_job[0] | ||
| to = module.scheduled_tasks["organisations_sync"].aws_cloudwatch_event_rule.this | ||
| } | ||
|
|
||
| moved { | ||
| from = aws_cloudwatch_event_target.ecs_org_sync_job[0] | ||
| to = module.scheduled_tasks["organisations_sync"].aws_cloudwatch_event_target.this | ||
| } |
There was a problem hiding this comment.
The moved blocks assume that the old resources existed (i.e., the features were enabled with count=1), but in environments where enable_organisations_sync or enable_mailchimp_sync were false, these resources never existed. When applying this change to those environments, Terraform will try to move non-existent resources and then create new ones, which should work but may cause state inconsistencies. Additionally, since the new module creates resources unconditionally, this means features that were previously disabled will suddenly become enabled in environments like staging and user-research.
| @@ -0,0 +1,80 @@ | |||
| variable "task_name" { | |||
| type = string | |||
| description = "The scheduled task name suffix." | |||
There was a problem hiding this comment.
The description says "The scheduled task name suffix" but this variable is actually used as the complete task family name, not just a suffix. Looking at the usage in main.tf line 5 and line 18 in scheduled-tasks.tf where it's set to "forms-admin-${replace(each.key, "_", "-")}", it's clear this is the full name. The description should be updated to "The complete task family name for the scheduled task" or similar to accurately reflect its usage.
| description = "The scheduled task name suffix." | |
| description = "The complete task family name for the scheduled task." |
| resource "aws_cloudwatch_event_rule" "sync_orgs_cron_job_failed" { | ||
| count = var.enable_organisations_sync ? 1 : 0 |
There was a problem hiding this comment.
The monitoring resources for orgs sync failures have lost their conditional creation. Previously, these resources had count = var.enable_organisations_sync ? 1 : 0 which prevented them from being created when organisations sync was disabled. Now these resources are always created, which means EventBridge will be monitoring for a task that may not exist in environments where enable_organisations_sync is false. This will create unnecessary resources and potentially confusing monitoring alerts.
| resource "aws_cloudwatch_event_rule" "sync_mailchimp_cron_job_failed" { | ||
| name = "${var.env_name}-forms-admin-mailchimp-sync-failed" |
There was a problem hiding this comment.
Similar to the organisations sync monitoring, the mailchimp sync failure monitoring resources have lost their conditional creation. Previously, these had count = var.enable_mailchimp_sync ? 1 : 0. Now they're always created even when mailchimp sync is disabled in the environment, leading to monitoring of non-existent tasks and unnecessary resource creation.
| module "scheduled_tasks" { | ||
| for_each = local.scheduled_tasks |
There was a problem hiding this comment.
The scheduled tasks are now created unconditionally, but the variables enable_organisations_sync and enable_mailchimp_sync still exist and are being passed to this module. The old implementation used these variables with count to conditionally create the resources. This change means that both scheduled tasks will always be created in all environments, regardless of the variable values, which could lead to unintended task executions in environments where these syncs should be disabled (e.g., staging and user-research environments have both set to false in their tfvars).
6491073 to
3d5e0a6
Compare
This is a shared IAM role for ECS events, this is needed by EventBridge to run ECS scheduled tasks.
3d5e0a6 to
a8b4967
Compare
Introduce a new module for ECS scheduled tasks, including task definition, CloudWatch event rule, and event target configuration. This makes it easy for us to define multiple ECS scheduled tasks.
This provides a place to easily define the schedule tasks.
a8b4967 to
150486a
Compare
What problem does this pull request solve?
This makes it easier to define multiple ECS Scheduled tasks (e.g. cron jobs). This adds a generic module that defines all the resources to create a scheduled task. This also creates a per account IAM role used by EventBridge scheduler to run tasks.
Will refactor the mailchimp and org sync task in a later PR.