-
Notifications
You must be signed in to change notification settings - Fork 10
Update postgres version, infra creation failing error throwing is enabled and enabled monitoring #126
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThree infrastructure and deployment configuration updates: CI/CD workflow error handling modified to fail on Terraform errors, monitoring Helmfile re-enabled via uncomment, and PostgreSQL version dependency upgraded from 15.8 to 15.12. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/digit_install.yml(1 hunks)devops/deploy-as-code/digit-helmfile.yaml(1 hunks)devops/infra-as-code/terraform/sample-aws/variables.tf(1 hunks)
🔇 Additional comments (2)
devops/deploy-as-code/digit-helmfile.yaml (1)
18-18: Monitoring helmfile configuration verified—no issues found.All referenced files exist and are properly configured:
./charts/monitoring/monitoring-helmfile.yaml✓- Environment files (
env-secrets.yaml,env.yaml) ✓- Values file (
loki-stack.yaml) ✓The helmfile structure is valid with proper Helm repositories and release definitions. No conflicts or missing dependencies detected.
devops/infra-as-code/terraform/sample-aws/variables.tf (1)
57-60: Verify application compatibility with PostgreSQL 15.12 libpq security fix before deployment.The upgrade from 15.8 to 15.12 is a safe in-place patch-level migration within the same 15.x minor version. However, PostgreSQL 15.12 includes a libpq security fix (CVE-2025-1094) that changes quoting function behavior: PQescapeLiteral(), PQescapeIdentifier(), and related functions now correctly honor the string-length parameter, whereas they previously ignored it.
Before proceeding with the upgrade:
- Audit the application code and any dependencies for direct usage of these libpq quoting functions.
- Applications that called these functions expecting the old broken behavior or that passed truncated buffers should be reviewed and tested.
- Confirm that the deployment strategy (blue-green, rolling, etc.) is appropriate for this patch and that rollback is available if needed.
| - name: Terraform Apply - remotestate | ||
| id: apply | ||
| run: terraform apply -no-color -input=false -auto-approve | ||
| continue-on-error: true | ||
| continue-on-error: false | ||
| working-directory: devops/infra-as-code/terraform/sample-aws/remote-state |
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.
Inconsistent error handling between remotestate and main infra apply steps.
Line 158 sets continue-on-error: false to fail on remotestate Terraform errors, but line 179 sets continue-on-error: true for main infra apply. This asymmetry could mask real infrastructure failures during main apply, allowing the workflow to proceed with incomplete infrastructure and potentially fail silently in downstream steps.
Clarify the intent: Should both steps use the same error handling strategy? If main infra apply errors are acceptable to ignore, add explicit checks in subsequent steps to verify infrastructure readiness before proceeding to deployment.
Consider aligning error handling:
- name: Terraform Apply - Infra creation
id: apply-Infra
run: terraform apply -no-color -input=false -auto-approve -var db_password=$db_password
- continue-on-error: true
+ continue-on-error: false
working-directory: devops/infra-as-code/terraform/sample-awsOr, if continuing on error is intentional, add a verification step:
- name: Verify Terraform Apply succeeded
run: |
if [ "${{ steps.apply-Infra.outcome }}" != "success" ]; then
echo "WARNING: Infra apply failed but continuing. Verify infrastructure state."
fi🤖 Prompt for AI Agents
.github/workflows/digit_install.yml lines 155-159: the Terraform remotestate
apply step uses continue-on-error: false while the main infra apply later uses
continue-on-error: true, creating inconsistent error handling that can mask real
failures; either make both steps use the same policy (set continue-on-error:
false for main infra apply to fail fast) or, if continuing on main apply is
intentional, add an explicit verification step immediately after the main infra
apply that checks the apply step outcome (using the main apply step id/outcome
or exit code) and fails or alerts if it did not succeed so downstream steps only
run when infra is verified.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.