feat(ecs): split queue and scheduler from web #7
Conversation
Summary of ChangesHello @leek, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a significant architectural enhancement by decoupling the web, queue worker, and scheduler components into separate AWS ECS services. This separation improves the application's scalability, resilience, and resource isolation. Alongside this core change, the PR introduces more flexible and diagnostic-rich GitHub Actions deployment workflows, automates changelog updates via pull requests, and refines Docker and Terraform configurations to support the new multi-service deployment model. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR restructures the ECS deployment to split the web, queue worker, and scheduler services into separate ECS services, improving scalability and isolation. It also enhances the deployment workflow with better diagnostics, multi-service support, and improved changelog automation.
Key Changes:
- Split queue worker and scheduler into dedicated ECS services with separate task definitions and supervisor configurations
- Enhanced deployment workflow to support multiple ECS services with improved stabilization checks and diagnostics
- Overhauled changelog update workflow to create PRs instead of direct commits, with better release note handling
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| terraform/modules/security/main.tf | Fixed duplicate variable name in IAM user naming |
| terraform/modules/compute/variables.tf | Added feature flags for enabling/disabling queue worker and scheduler services |
| terraform/modules/compute/outputs.tf | Added outputs for new worker service names |
| terraform/modules/compute/main.tf | Refactored to create separate ECS services for web, queue-worker, and scheduler with shared configuration |
| docker/supervisord.conf | Removed queue worker and scheduler configurations (moved to dedicated files) |
| docker/supervisord-web.conf | New supervisor config for web service only (nginx + php-fpm) |
| docker/supervisord-scheduler.conf | New supervisor config dedicated to Laravel scheduler |
| docker/supervisord-queue-worker.conf | New supervisor config dedicated to queue workers |
| docker/README.md | Updated documentation to reflect multi-service architecture and container roles |
| docker/Dockerfile | Added new supervisor configuration files for each service role |
| .github/workflows/update-changelog.yml.example | Refactored to trigger on deployment completion, create PRs, and handle releases more robustly |
| .github/workflows/fix-code-style.yml.example | Improved checkout configuration and added explicit branch specification |
| .github/workflows/deploy.yml.example | Enhanced to deploy multiple ECS services with improved diagnostics and Teams notifications |
| .github/workflows/copilot-setup-steps.yml.example | New workflow demonstrating Copilot setup step validation |
Comments suppressed due to low confidence (1)
terraform/modules/compute/main.tf:1
- Operator precedence issue in bash condition. The
||will bind more loosely than&&, causing unexpected evaluation. Wrap the first condition in parentheses:([ "$EXIT_CODE" != "0" ] && [ "$EXIT_CODE" != "N/A" ]) || [ "$STOP_CODE" = "TaskFailedToStart" ]
# ========================================
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to the Docker configuration, ECS deployment, and related workflows. The changes include splitting the queue and scheduler from the web service, enhancing the Dockerfile to support multiple container roles, and updating the Terraform configuration to manage the new ECS services. The addition of supervisor configurations for web, queue worker, and scheduler processes in the Dockerfile, along with corresponding updates in the Terraform modules, enables a more modular and scalable deployment architecture. The changes to the README.md provide clear instructions on how to use the new container roles and services.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughDecouples web, queue-worker, and scheduler into separate Docker supervisor configs and ECS tasks/services; updates Dockerfile, docs, GitHub Actions examples for multi-service deploys; refactors Terraform compute module to add worker task/service resources, outputs, and toggles; minor .gitignore/.dockerignore and IAM naming changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant ECR as ECR
participant ECS as ECS Cluster
participant TF as Terraform
rect rgba(120,180,240,0.12)
Note over GH,ECR: Build & push single image (APP_NAME-ENV)
GH->>ECR: Build & Push image
end
rect rgba(160,220,160,0.12)
Note over GH,ECS: Multi-service Deploy Flow
GH->>ECS: For each service in ECS_SERVICES -> Update service (main, queue-worker, scheduler)
GH->>ECS: Poll for stabilization (max attempts / interval)
alt stabilized
GH->>ECS: Confirm health checks
else failed
GH->>ECS: Fetch service status & stopped tasks
GH->>ECS: Fetch CloudWatch logs -> Diagnostics
GH->>GH: Send Teams failure notification
end
end
rect rgba(240,220,140,0.12)
Note over TF,ECS: Infra provisioning for workers
TF->>ECS: Create aws_ecs_task_definition.worker (for_each enabled services)
TF->>ECS: Create aws_ecs_service.worker (for_each enabled services)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (6)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
terraform/modules/compute/main.tf (1)
255-264: Deployment strategy improvements support better rolling updates for low task counts.Increasing
deployment_maximum_percentfrom 100 to 200 (line 259) withdeployment_circuit_breakerenabled allows proper rolling deploys when desired count is low (1–2 tasks), preventing deployment hangs due to ENI constraints. The 50% minimum healthy percent balances cost and resilience. Well documented in the preceding comments.However, note that
deployment_maximum_percentis now a hardcoded value. If future deployments require tuning this strategy per environment, consider extracting it as a variable..github/workflows/fix-code-style.yml.example (1)
41-41: Upgrade setup-node to v4 and enable caching.
Use latest action and cache npm to speed runs.- - name: Setup NodeJS - uses: actions/setup-node@v2 + - name: Setup NodeJS + uses: actions/setup-node@v4 + with: + cache: npm + # optionally pin a version or use .nvmrc: + # node-version-file: .nvmrc.dockerignore (1)
17-21: Ignore vendor/ to shrink build context.
Composer installs in-image; excluding vendor/ avoids large uploads.# Development files .env* !.env.example *.log npm-debug.log yarn-error.log .phpunit.result.cache Homestead.json Homestead.yaml .claude .husky +vendor/.github/workflows/update-changelog.yml.example (1)
61-97: Guard parsing when release notes are empty.
Short-circuit to avoid creating empty sections.- name: Update CHANGELOG.md env: VERSION: ${{ steps.rel.outputs.tag }} RELEASE_NOTES: ${{ steps.rel.outputs.body }} run: | set -e + if [ -z "${VERSION}" ]; then + echo "No version resolved; exiting." + exit 0 + fi.github/workflows/deploy.yml.example (4)
43-51: Map the 'uat' branch explicitly.
on.push.branchesincludesuat, but the env step maps anything notproduction/stagingtostaging. Add auatcase to avoid mislabeling.- if [[ ${{ github.ref }} == 'refs/heads/production' ]]; then + if [[ ${{ github.ref }} == 'refs/heads/production' ]]; then echo "environment=production" >> $GITHUB_OUTPUT elif [[ ${{ github.ref }} == 'refs/heads/staging' ]]; then echo "environment=staging" >> $GITHUB_OUTPUT + elif [[ ${{ github.ref }} == 'refs/heads/uat' ]]; then + echo "environment=uat" >> $GITHUB_OUTPUT else echo "environment=staging" >> $GITHUB_OUTPUT fi
184-205: Diagnostics only target the web service. Include all services.If stabilization fails, you gather details for
${APP_NAME}-${ENVIRONMENT}-serviceonly. Extend diagnostics to iterate over all configured services so worker/scheduler failures are visible.IFS=',' read -ra SERVICE_SUFFIXES <<< "${{ env.ECS_SERVICES }}" for SUFFIX in "${SERVICE_SUFFIXES[@]}"; do SVC="${{ env.APP_NAME }}-${ENVIRONMENT}-${SUFFIX}" echo "📊 Service Status: $SVC" aws ecs describe-services --cluster "${{ env.APP_NAME }}-${ENVIRONMENT}" --services "$SVC" \ --query 'services[0].{Status:status,Running:runningCount,Desired:desiredCount,Pending:pendingCount,Deployments:deployments[*].{Status:rolloutState,Running:runningCount,Desired:desiredCount}}' \ --output table done
316-333: Container selection anddown/upsemantics.
- Picking the first container may target a sidecar. Allow override:
-CONTAINER_NAME=$(echo "$TD_JSON" | jq -r '.taskDefinition.containerDefinitions[0].name') +CONTAINER_NAME="${ECS_CONTAINER_NAME:-$(echo "$TD_JSON" | jq -r '.taskDefinition.containerDefinitions[0].name')}"
- Verify that
php artisan down/upacts on shared storage (e.g., EFS). If web tasks use ephemeral storage, maintenance mode won’t affect live traffic and can be removed.
336-347: Tag the one-off task with startedBy for traceability.Add
--started-by "github-actions-post-deploy"toaws ecs run-taskso it’s easy to trace in ECS.
♻️ Duplicate comments (6)
terraform/modules/compute/main.tf (1)
349-354: CONTAINER_ROLE concatenation is safe but verify it won't conflict with additional variables.The
CONTAINER_ROLEenvironment variable is prepended tolocal.common_environment_variables(which does not currently defineCONTAINER_ROLE). However, ifCONTAINER_ROLEis ever added tovar.additional_environment_variables, there will be a duplicate. Consider filtering it explicitly as suggested in prior feedback, or document the constraint.environment = concat([ { name = "CONTAINER_ROLE" value = each.value.container_role } ], [for env in local.common_environment_variables : env if env.name != "CONTAINER_ROLE"])Alternatively, verify that
CONTAINER_ROLEis always managed centrally and never included inadditional_environment_variables..github/workflows/fix-code-style.yml.example (1)
56-56: Resolved: dynamic target branch.
This addresses the prior concern about hardcoding to main. Good improvement.docker/supervisord-queue-worker.conf (1)
9-20: Parameterize worker flags via environment; avoid hardcoded SQS options.
Makes tuning possible without image rebuilds and matches infra-as-config goals.[program:queue-worker] process_name=%(program_name)s_%(process_num)02d -command=php /var/www/html/artisan queue:work sqs --sleep=3 --tries=3 --max-time=3600 --timeout=90 +; Defaults can be overridden by container env +environment=QUEUE_CONNECTION="sqs",QUEUE_SLEEP="3",QUEUE_TRIES="3",QUEUE_MAX_TIME="3600",QUEUE_TIMEOUT="90" +command=php /var/www/html/artisan queue:work %(ENV_QUEUE_CONNECTION)s --sleep=%(ENV_QUEUE_SLEEP)s --tries=%(ENV_QUEUE_TRIES)s --max-time=%(ENV_QUEUE_MAX_TIME)s --timeout=%(ENV_QUEUE_TIMEOUT)s user=www-data numprocs=2 autostart=true autorestart=true startretries=3 +startsecs=5 +stopwaitsecs=30 stdout_logfile=/dev/stdout stdout_logfile_maxbytes=0 stderr_logfile=/dev/stderr stderr_logfile_maxbytes=0 stopasgroup=true killasgroup=trueOptional: expose numprocs via build-time templating or envsubst at entrypoint.
.github/workflows/deploy.yml.example (1)
82-85: Comment clarity: force new deployments across all services.Consider noting that these commands force new deployments for web, queue worker, and scheduler services, to align with reader expectations.
docker/Dockerfile (1)
130-132: Add brief inline comments for each COPY.Document each per‑role config’s purpose to aid maintainers (web, queue‑worker, scheduler).
docker/supervisord-scheduler.conf (1)
8-16: Make interval configurable and preferschedule:workto avoid overlaps.
- Parameterize the sleep:
sleep ${SCHEDULER_SLEEP:-60}.- Consider replacing the loop with Laravel’s built‑in worker:
php /var/www/html/artisan schedule:work --verbose --no-interaction. It handles timing and avoids running a new process every cycle.-command=/bin/sh -c "while true; do php /var/www/html/artisan schedule:run --verbose --no-interaction; sleep 60; done" +command=/bin/sh -c "php /var/www/html/artisan schedule:work --verbose --no-interaction --no-ansi"If you keep the loop:
-command=/bin/sh -c "while true; do php /var/www/html/artisan schedule:run --verbose --no-interaction; sleep 60; done" +command=/bin/sh -c "while true; do php /var/www/html/artisan schedule:run --verbose --no-interaction; sleep ${SCHEDULER_SLEEP:-60}; done"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
.dockerignore(1 hunks).envrc(1 hunks).github/workflows/copilot-setup-steps.yml.example(1 hunks).github/workflows/deploy.yml.example(6 hunks).github/workflows/fix-code-style.yml.example(2 hunks).github/workflows/update-changelog.yml.example(2 hunks).gitignore(1 hunks)docker/Dockerfile(1 hunks)docker/README.md(3 hunks)docker/supervisord-queue-worker.conf(1 hunks)docker/supervisord-scheduler.conf(1 hunks)docker/supervisord-web.conf(1 hunks)docker/supervisord.conf(0 hunks)terraform/.gitignore(1 hunks)terraform/modules/compute/main.tf(4 hunks)terraform/modules/compute/outputs.tf(1 hunks)terraform/modules/compute/variables.tf(1 hunks)terraform/modules/security/main.tf(2 hunks)
💤 Files with no reviewable changes (1)
- docker/supervisord.conf
🧰 Additional context used
🪛 LanguageTool
docker/README.md
[style] ~18-~18: ‘compete with each other’ might be wordy. Consider a shorter alternative.
Context: ...paration ensures: - Queue workers don't compete with each other (no duplicate job processing) - Only on...
(EN_WORDINESS_PREMIUM_COMPETE_WITH_EACH_OTHER)
🔇 Additional comments (12)
.gitignore (1)
78-79: Deployment and Laravel Boost artifacts are now properly ignored.The additions of
/VERSION(deployment-generated version file) and Laravel Boost configuration artifacts (.mcp.json,.junie/,.claude/settings.local.json) align with the enhanced deployment workflow and local development tooling. No concerns.Also applies to: 91-94
terraform/modules/security/main.tf (1)
419-419: IAM resource naming is now Laravel-specific and consistent.Renaming from generic "-user" to "-laravel-user" and "-laravel-user-policy" improves clarity and aligns with the broader module naming convention. Resource identifiers and tags are consistently updated.
Also applies to: 422-422, 433-433
terraform/modules/compute/outputs.tf (1)
32-45: New worker service outputs are correctly conditioned and map-aware.The outputs
queue_worker_service_nameandscheduler_service_nameproperly use ternary operators to returnnullwhen disabled, andworker_servicesexposes the full map for downstream reference. Map key access (queue-worker,scheduler) aligns with thelocal.worker_servicesconfiguration in main.tf.Please confirm that the map keys ("queue-worker", "scheduler") in the worker services outputs match the keys defined in
terraform/modules/compute/main.tflines 119–142, and that no downstream consumers of these outputs depend on a specific key naming convention.terraform/modules/compute/variables.tf (1)
141-151: New worker service enable variables are well-defined and backward-compatible.The
enable_queue_workerandenable_schedulervariables have clear descriptions and sensible defaults (bothtrue), which maintains backward compatibility while allowing optional opt-out. Naming and type are consistent with project conventions.terraform/modules/compute/main.tf (3)
5-148: Locals refactoring improves configuration reusability and clarity.Centralizing
common_environment_variables,common_secrets, andworker_servicesinto a locals block reduces duplication and makes the module easier to maintain. Theenabled_worker_servicesfilter elegantly gates which worker services are instantiated based on the new enable flags. Well structured.
212-213: Main task definition correctly uses centralized environment and secret locals.The main ECS task now references
local.common_environment_variablesandlocal.common_secrets, ensuring consistency with worker services and reducing duplication. Clean refactor.
325-412: Worker services are well-structured with flexible, scalable configuration.The new
aws_ecs_task_definition.workerandaws_ecs_service.workerresources usefor_eachoverlocal.enabled_worker_services, enabling both queue-worker and scheduler with independent configuration (CPU, memory, desired count, logging prefix). The reuse oflocal.common_environment_variablesandlocal.common_secretsensures consistency. ECS Exec is enabled for debugging, and the optionaldeployment_circuit_breaker(lines 401–407) is properly guarded.Note: The current
worker_servicesmap specifiesdeployment_config = {}(empty), so the circuit breaker is not enabled for these services. This is acceptable, but if circuit breaker is needed later, update the config inline or extract as variables.Please verify that:
- The
for_eachkeys match outputs interraform/modules/compute/outputs.tf(confirmed: "queue-worker" and "scheduler").- Worker services do not need
load_balancerblocks (queue and scheduler are not fronted by ALB, correct).- ECS Exec is sufficient for debugging; no additional logging or monitoring changes are needed.
.github/workflows/fix-code-style.yml.example (1)
25-25: LGTM: full history is appropriate for formatting-only commits..github/workflows/copilot-setup-steps.yml.example (1)
8-12: Paths filter targets the non-example file. Confirm intent.
This example won’t trigger on changes to itself. Ensure a real .github/workflows/copilot-setup-steps.yml exists or adjust filters..github/workflows/update-changelog.yml.example (2)
20-25: Branch gate hardcoded to 'production'. Verify branch names.
Deploy workflow must run on production for this to trigger; checkout targets main. Confirm this matches your release flow.
141-152: Expose a changed output even when file unchanged; add no-op path.
Prevents later steps from failing on missing outputs.- name: Commit changes locally (if any) id: commit run: | git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" if git diff --quiet CHANGELOG.md; then echo "changed=false" >> $GITHUB_OUTPUT else git add CHANGELOG.md git commit -m "chore(changelog): update for release ${{ steps.rel.outputs.tag }}" echo "changed=true" >> $GITHUB_OUTPUT fi + shell: bashLikely an incorrect or invalid review comment.
.github/workflows/deploy.yml.example (1)
108-124: Cluster and service naming confirmed as correct.Verification of Terraform configuration confirms the deploy.yml naming assumptions are accurate. Terraform defines cluster as
"${var.app_name}-${var.environment}"(line 159,terraform/modules/compute/main.tf), main service as"${var.app_name}-${var.environment}-service"(line 241), and worker services as"${var.app_name}-${var.environment}-${each.key}"(line 381). These patterns align exactly with the script's resource references.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fixes #3
This pull request introduces several improvements to the GitHub Actions workflows and deployment configuration, focusing on increased flexibility, better diagnostics, and improved automation for changelog updates. The most significant changes include enhanced ECS deployment logic, improved error notifications, a more robust changelog update workflow, and additional configuration options for the deployment process.
Deployment Workflow Improvements:
.github/workflows/deploy.yml.exampleto support deploying multiple ECS services, configurable via the newECS_SERVICESandAPP_NAMEenvironment variables, and generalized Docker image/tag handling for improved flexibility. [1] [2]Changelog Automation:
.github/workflows/update-changelog.yml.exampleto trigger on successful deployments or manual dispatch, resolve releases by tag or latest, and create a pull request for changelog updates instead of pushing directly to production. This workflow also ensuresCHANGELOG.mdexists and merges unreleased and release note content more robustly. [1] [2]Dockerfile Enhancements:
docker/Dockerfile, supporting multi-service deployments.Other Workflow Tweaks:
.github/workflows/fix-code-style.yml.example) by using full fetch depth for checkouts and specifying the branch for auto-commits. [1] [2]New Example Workflow:
.github/workflows/copilot-setup-steps.yml.exampleto demonstrate Copilot setup steps validation and manual testing.Summary by CodeRabbit
New Features
Documentation
Chores