-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding Readiness checks and upscaling instances to handle Website deployments #2415
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhanced deployment configurations for the Changes
Sequence DiagramsequenceDiagram
participant Workflow as Deployment Workflow
participant App as Website2
Workflow->>App: Increase min instances to 2
Workflow->>App: Apply Readiness Checks
App-->>Workflow: Deployment Readiness Status
Workflow->>App: Restore min instances to 1
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/deploy-frontends-to-production.yml (1)
304-308
: Add error handling for instance scaling operations.While the instance scaling strategy is good for maintaining availability during deployment, the implementation lacks error handling.
Consider adding error handling and verification:
- name: Temporarily Increase Min Instances run: | echo "========== Setting minimum instances to 2 for deployment ==========" - gcloud app services update default --min-instances=2 --quiet + if ! gcloud app services update default --min-instances=2 --quiet; then + echo "Failed to increase instances. Proceeding with deployment..." + fi + # Verify the change + actual_instances=$(gcloud app services describe default --format='get(basicScaling.minInstances)') + echo "Current min instances: $actual_instances" # ... deployment steps ... - name: Restore Min Instances run: | echo "========== Restoring minimum instances to 1 after deployment ==========" - gcloud app services update default --min-instances=1 --quiet + if ! gcloud app services update default --min-instances=1 --quiet; then + echo "Failed to restore instances. Manual intervention may be required." + exit 1 + fi + # Verify the change + actual_instances=$(gcloud app services describe default --format='get(basicScaling.minInstances)') + echo "Current min instances: $actual_instances"Also applies to: 329-332
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/deploy-frontends-to-production.yml (1)
304-308
: Consider adding error handling for instance scaling operations.While the instance scaling strategy is good for maintaining availability during deployment, the scaling operations should include error handling to ensure the instance count is properly restored even if the deployment fails.
- name: Temporarily Increase Min Instances run: | echo "========== Setting minimum instances to 2 for deployment ==========" - gcloud app services update default --min-instances=2 --quiet + if ! gcloud app services update default --min-instances=2 --quiet; then + echo "Failed to increase instances. Proceeding with deployment..." + fi # ... deployment steps ... - name: Restore Min Instances run: | echo "========== Restoring minimum instances to 1 after deployment ==========" - gcloud app services update default --min-instances=1 --quiet + if ! gcloud app services update default --min-instances=1 --quiet; then + echo "::warning::Failed to restore instance count to 1. Manual intervention may be required." + exit 1 + fiAlso applies to: 329-332
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy-frontends-to-production.yml
(3 hunks)
🔇 Additional comments (2)
.github/workflows/deploy-frontends-to-production.yml (2)
289-294
: Thank you for implementing the suggested readiness check parameters!The readiness check configuration looks good with the recommended values from the previous review.
Line range hint
304-332
: Verify the cool-down period alignment.The
cool_down_period_sec
is set to 80 seconds in the app.yaml configuration. Ensure this aligns with the instance scaling operations during deployment to prevent potential conflicts between automated scaling and manual scaling operations.
Summary of Changes (What does this PR do?)
Summary by CodeRabbit
New Features
website2
with advanced readiness checks.Improvements