updated deploy workflow #2740
updated deploy workflow #2740
Conversation
The GitHub action is updated to use currently active ecs task definition instead of the one defined in the repo. This ensures changes made in ecs cluster stay there instead of getting reset.
📝 WalkthroughWalkthroughThe pull request involves streamlining the deployment workflow configuration by removing several AWS-related environment variables and task definition files. The GitHub Actions workflow for deployment has been simplified, with changes to how task definitions are retrieved and processed. The Changes
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2740 +/- ##
========================================
Coverage 60.46% 60.46%
========================================
Files 252 252
Lines 12705 12705
Branches 1111 1111
========================================
Hits 7682 7682
Misses 4954 4954
Partials 69 69 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/deploy.yml (3)
81-85: Perhaps we could skip the manual cache directory creation?The
docker/build-push-actiontypically handles cache directory creation automatically. These manual mkdir commands seem a bit... redundant, don't you think?- - name: Create new cache - run: | - mkdir -p /tmp/.buildx-cache - mkdir -p /tmp/.buildx-cache-new
109-115: The cache update could use some error handling, just saying...While running this step even on failure is clever, we might want to add some basic error handling and logging.
- name: Update cache if: always() run: | + set -e if [ -d "/tmp/.buildx-cache-new" ]; then + echo "Updating build cache..." rm -rf /tmp/.buildx-cache mv /tmp/.buildx-cache-new /tmp/.buildx-cache + echo "Cache update completed successfully" + else + echo "No new cache directory found, skipping update" fi
165-191: A rollback strategy would be nice, wouldn't it?While the deployment steps look good, we might want to add a rollback strategy in case things go sideways. Just a thought.
Consider adding:
- Task definition version tracking
- Automatic rollback on deployment failure
- Manual rollback capability through workflow dispatch
Example rollback step:
- name: Rollback on failure if: failure() run: | aws ecs update-service \ --cluster ${{ env.ECS_CLUSTER }} \ --service ${{ env.ECS_SERVICE_BACKEND }} \ --task-definition ${{ env.ECS_SERVICE_BACKEND }}:${{ steps.get-previous-task-definition.outputs.version }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/deploy.yml(4 hunks)aws/backend.json(0 hunks)aws/celery.json(0 hunks)
💤 Files with no reviewable changes (2)
- aws/backend.json
- aws/celery.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (2)
.github/workflows/deploy.yml (2)
173-176: The Backend task definition download has the same... opportunities for improvement.This section could benefit from the same robustness improvements suggested for the Celery task definition download.
Please apply the same improvements suggested for the Celery task definition download above.
157-163: Might want to validate that task definition before deploying.The task definition update looks fine, but we should probably validate the rendered output before proceeding.
.github/workflows/deploy.yml
Outdated
| - name: Download task definition for Celery Service | ||
| run: | | ||
| aws ecs describe-task-definition --task-definition ${{ env.ECS_SERVICE_CELERY }} --query taskDefinition > celery-task-definition.json | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
The task definition download could be more... robust.
While fetching the current task definition is a good approach, we might want to add error handling and be more specific with our query.
- name: Download task definition for Celery Service
run: |
+ set -e
+ echo "Downloading Celery task definition..."
- aws ecs describe-task-definition --task-definition ${{ env.ECS_SERVICE_CELERY }} --query taskDefinition > celery-task-definition.json
+ aws ecs describe-task-definition --task-definition ${{ env.ECS_SERVICE_CELERY }} \
+ --query 'taskDefinition' \
+ --output json > celery-task-definition.json || {
+ echo "Failed to download Celery task definition"
+ exit 1
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Download task definition for Celery Service | |
| run: | | |
| aws ecs describe-task-definition --task-definition ${{ env.ECS_SERVICE_CELERY }} --query taskDefinition > celery-task-definition.json | |
| - name: Download task definition for Celery Service | |
| run: | | |
| set -e | |
| echo "Downloading Celery task definition..." | |
| aws ecs describe-task-definition --task-definition ${{ env.ECS_SERVICE_CELERY }} \ | |
| --query 'taskDefinition' \ | |
| --output json > celery-task-definition.json || { | |
| echo "Failed to download Celery task definition" | |
| exit 1 | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/deploy.yml (3)
7-7: Would be nice to document the removal of staging branch trigger.I see we're quietly removing the staging branch trigger. While I'm sure there's a perfectly good reason for this, perhaps we could add a comment explaining why, just so future maintainers don't waste their time wondering about it?
81-85: The cache management could use a bit more... finesse.While the cache creation and update logic is a step in the right direction, we might want to consider:
- Adding cleanup of old caches to prevent unlimited growth
- Setting size limits for the cache
- Adding error handling for directory operations
- name: Create new cache run: | + set -e mkdir -p /tmp/.buildx-cache mkdir -p /tmp/.buildx-cache-new + # Cleanup old caches (older than 7 days) + find /tmp/.buildx-cache* -type f -mtime +7 -delete || true + # Set size limit (e.g., 5GB) + cache_size=$(du -sb /tmp/.buildx-cache | cut -f1) + if [ "$cache_size" -gt 5368709120 ]; then + echo "Cache size exceeds 5GB, cleaning oldest files..." + find /tmp/.buildx-cache -type f -printf '%T+ %p\n' | sort | head -n 100 | cut -d' ' -f2- | xargs rm + fiAlso applies to: 109-115
159-165: The task definition rendering steps look... familiar.I see we're using the same pattern for both services. Perhaps we could make this more DRY?
Consider creating a reusable job:
deploy-service: inputs: service-name: required: true container-name: required: true steps: - uses: ./.github/actions/download-task-definition with: service-name: ${{ inputs.service-name }} output-file: task-definition.json - uses: aws-actions/amazon-ecs-render-task-definition@v1 with: task-definition: task-definition.json container-name: ${{ inputs.container-name }} image: ${{ env.IMAGE_VALUE }}Also applies to: 184-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
.github/workflows/deploy.yml (1)
149-198: Changes align well with the PR objectives.The implementation successfully moves from predefined to active ECS task definitions, which should help preserve cluster changes as intended. The error handling, while basic, should prevent most deployment issues.
The GitHub action is updated to use currently active ecs task definition instead of the one defined in the repo. This ensures changes made in ecs cluster stay there instead of getting reset.
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Deployment Configuration
Infrastructure
Note: These changes primarily affect the deployment infrastructure and do not introduce visible end-user features.