-
Notifications
You must be signed in to change notification settings - Fork 9
Migration: Auto-run migrations on staging deployment #592
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: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe backend container CMD was changed to run a startup script; the new script runs alembic migrations (upgrade head) then execs uvicorn to start the app on 0.0.0.0:80 with 4 workers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Container
participant StartScript as "start.sh"
participant Alembic
participant Database
participant Uvicorn as "uvicorn (app.main)"
Container->>StartScript: CMD runs bash scripts/start.sh
StartScript->>Alembic: run "uv run alembic upgrade head"
Alembic->>Database: apply migrations
Database-->>Alembic: migration result
StartScript->>Uvicorn: exec "uv run uvicorn app.main:app --host 0.0.0.0 --port 80 --workers 4"
Uvicorn-->>Container: serve requests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@backend/scripts/start.sh`:
- Around line 1-6: The script missing a trailing newline at EOF is breaking the
end-of-file pre-commit hook; open the start.sh script (look for the shebang and
commands like "uv run alembic upgrade head" and "exec uv run uvicorn
app.main:app --host 0.0.0.0 --port 80 --workers 4") and add a single newline
character at the end of the file so the file ends with a newline.
🧹 Nitpick comments (1)
backend/scripts/start.sh (1)
4-4: Migration race condition when running multiple container replicas.If the service is scaled beyond a single task/replica (now or in the future), every container will attempt
alembic upgrade headconcurrently on startup. Alembic does not acquire an advisory lock by default, so concurrent migrations can fail or corrupt thealembic_versiontable.This is fine for a single-instance staging deployment today, but consider one of these guards before scaling or promoting to production:
- Use a PostgreSQL advisory lock in
env.py(SELECT pg_advisory_lock(...)before running migrations).- Run migrations as a separate one-off ECS task / init container rather than on every replica startup.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
LGTM
Summary
Target issue is #237
Explain the motivation for making this change. What existing problem does the pull request solve?
Added a startup script (start.sh) that runs alembic upgrade head before starting uvicorn, and updated the Dockerfile CMD to use it. so that there is no need to manually run migration.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Tested locally using docker run to simulate ECS behavior, migrations ran successfully (001→044) before app startup
Summary by CodeRabbit