feat(migrations): implement Alembic for database migrations (#2)#563
Merged
nanotaboada merged 10 commits intomasterfrom Apr 10, 2026
Merged
feat(migrations): implement Alembic for database migrations (#2)#563nanotaboada merged 10 commits intomasterfrom
nanotaboada merged 10 commits intomasterfrom
Conversation
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds async Alembic migration infrastructure with three migrations (create Changes
Sequence DiagramsequenceDiagram
participant Entrypoint as Entrypoint Script
participant Gunicorn as Gunicorn Master
participant Alembic as Alembic CLI/Config
participant Engine as Async DB Engine
participant DB as Database (SQLite/Postgres)
participant App as FastAPI Worker
Entrypoint->>Gunicorn: exec gunicorn (reads config)
Gunicorn->>Alembic: on_starting -> run `alembic upgrade head`
Alembic->>Engine: create async engine (pool.NullPool)
Engine->>DB: open connection / begin transaction
Alembic->>DB: apply migrations (001 → 002 → 003)
Engine->>DB: commit / close connection
Alembic-->>Gunicorn: migrations complete
Gunicorn->>App: fork workers (App starts serving)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #563 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 7 +4
Lines 114 172 +58
=========================================
+ Hits 114 172 +58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
alembic/env.py (1)
25-27: Consider extracting shared DATABASE_URL logic.The fallback logic for constructing
DATABASE_URLis duplicated between this file anddatabases/player_database.py(lines 22-27). While they're currently identical, this could drift over time.You could extract this to a shared utility, though the duplication is acceptable given Alembic's standalone execution requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@alembic/env.py` around lines 25 - 27, The duplicated DATABASE_URL construction (_storage_path, _default_url, database_url) should be extracted into a single helper function (e.g., get_database_url or build_database_url) that encapsulates reading STORAGE_PATH and DATABASE_URL and returning the final URL; update both places that define _storage_path/_default_url/database_url to call that helper (replace the duplicated logic in functions where these symbols appear) so the behavior is centralized and future drift is prevented while keeping Alembic-compatible import patterns (make the helper import-safe for standalone execution).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 39-41: The lifespan() hook currently calls _apply_migrations via
loop.run_in_executor (logger.info + await loop.run_in_executor(None,
_apply_migrations)), which will run once per FastAPI process and cause
concurrent Alembic upgrades; remove this direct call from lifespan() and either
(A) move invocation of _apply_migrations out of app startup into a one-shot
initialization step (entrypoint script, deployment job, or init container) or
(B) guard the migration function by implementing a DB-backed lock around
_apply_migrations (e.g., an advisory/locking table or transactional lock) so
only one process executes migrations; update any logging (logger.info) to
reflect the chosen approach and keep function name _apply_migrations as the
guarded/moved target.
- Around line 25-27: The call in _apply_migrations uses Config("alembic.ini")
which is cwd-dependent; change it to resolve alembic.ini relative to main.py so
migrations work regardless of working directory: compute the package file
location (e.g. Path(__file__).resolve().parent / "alembic.ini") and pass its
string path into Config, ensuring any required import (pathlib.Path) is added
and that the function _apply_migrations still calls command.upgrade(alembic_cfg,
"head") with the updated Config instance.
---
Nitpick comments:
In `@alembic/env.py`:
- Around line 25-27: The duplicated DATABASE_URL construction (_storage_path,
_default_url, database_url) should be extracted into a single helper function
(e.g., get_database_url or build_database_url) that encapsulates reading
STORAGE_PATH and DATABASE_URL and returning the final URL; update both places
that define _storage_path/_default_url/database_url to call that helper (replace
the duplicated logic in functions where these symbols appear) so the behavior is
centralized and future drift is prevented while keeping Alembic-compatible
import patterns (make the helper import-safe for standalone execution).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a79a3a98-8511-450d-aac6-e3df3f0c7465
⛔ Files ignored due to path filters (2)
storage/players-sqlite3.dbis excluded by!**/*.db,!**/storage/**,!**/*.dbuv.lockis excluded by!**/*.lock,!**/uv.lock
📒 Files selected for processing (15)
.gitignoreCHANGELOG.mdDockerfilealembic.inialembic/READMEalembic/env.pyalembic/script.py.makoalembic/versions/001_create_players_table.pyalembic/versions/002_seed_starting11.pyalembic/versions/003_seed_substitutes.pycompose.yamldatabases/player_database.pymain.pypyproject.tomlscripts/entrypoint.sh
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…#2) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
alembic/env.py (2)
49-56: Consider adding a brief docstring todo_run_migrations.While this is a standard Alembic helper pattern, a one-line docstring would improve readability and silence the static analysis hint.
Suggested fix
def do_run_migrations(connection: Connection) -> None: + """Configure Alembic context and run migrations within a transaction.""" context.configure( connection=connection, target_metadata=target_metadata, render_as_batch=True, ) with context.begin_transaction(): context.run_migrations()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@alembic/env.py` around lines 49 - 56, Add a one-line docstring to the do_run_migrations function describing its purpose (e.g., "Run Alembic migrations using the provided DB connection") to improve readability and satisfy static analysis; place it immediately after the def do_run_migrations(connection: Connection) -> None: line so the function now has a concise explanatory docstring.
59-67: Consider adding a brief docstring torun_async_migrations.Suggested fix
async def run_async_migrations() -> None: + """Create async engine, connect, and run migrations synchronously.""" connectable = async_engine_from_config( config.get_section(config.config_ini_section, {}), prefix="sqlalchemy.", poolclass=pool.NullPool, ) async with connectable.connect() as connection: await connection.run_sync(do_run_migrations) await connectable.dispose()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@alembic/env.py` around lines 59 - 67, Add a concise docstring to the async function run_async_migrations describing its purpose (run Alembic migrations against an async SQLAlchemy engine built from the alembic config), its behavior (creates async engine via async_engine_from_config, opens an async connection and calls do_run_migrations with connection.run_sync, then disposes the engine), and the return type (None); place it immediately under the def run_async_migrations() line so readers and tools can understand intent and side effects.tests/test_migrations.py (1)
24-24: Add return type hints to test functions.Per coding guidelines, type hints are required everywhere.
Suggested fix
-def test_migration_downgrade_003_removes_substitutes_only(): +def test_migration_downgrade_003_removes_substitutes_only() -> None: ... -def test_migration_downgrade_002_removes_starting11_only(): +def test_migration_downgrade_002_removes_starting11_only() -> None: ... -def test_migration_downgrade_001_drops_players_table(): +def test_migration_downgrade_001_drops_players_table() -> None:Also applies to: 39-39, 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_migrations.py` at line 24, Add missing return type annotations to the test functions (e.g., test_migration_downgrade_003_removes_substitutes_only and the other test functions at the indicated locations) so they follow project typing guidelines; update each test function signature to include -> None (e.g., def test_migration_downgrade_003_removes_substitutes_only() -> None:) and run the test/type checker to ensure no other typing issues remain.tests/conftest.py (1)
18-21: Add return type hint to the fixture.Per coding guidelines, type hints are required everywhere. The fixture should include
-> None.Suggested fix
`@pytest.fixture`(scope="session", autouse=True) -def apply_migrations(): +def apply_migrations() -> None: """Apply Alembic migrations once before the test session starts.""" command.upgrade(ALEMBIC_CONFIG, "head")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 18 - 21, The apply_migrations pytest fixture is missing a return type hint; update the function signature of apply_migrations (the pytest.fixture that calls command.upgrade with ALEMBIC_CONFIG) to include a -> None return type. Ensure the annotation is added to the def apply_migrations(...) line and keep the fixture behavior (calling command.upgrade(ALEMBIC_CONFIG, "head")) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_migrations.py`:
- Around line 52-64: The test test_migration_downgrade_001_drops_players_table
currently performs the assertion after command.upgrade, so move the assertion
that the players table is gone to immediately after you fetch the `table` value
following command.downgrade(ALEMBIC_CONFIG, "base") (i.e., assert `table is
None` before calling command.upgrade), or alternatively re-query the DB after
downgrade and then assert; ensure references to `command.downgrade`, the `table`
variable, and `command.upgrade` are updated accordingly so the test verifies the
downgrade result before performing the upgrade.
- Around line 39-49: The test
test_migration_downgrade_002_removes_starting11_only currently performs its
assertion after calling command.upgrade, so move the assertion checking total ==
0 to immediately after fetching/closing the DB connection (right after
conn.close() or immediately after computing total) and before calling
command.upgrade(ALEMBIC_CONFIG, "head"); keep the rest of the flow
(command.downgrade and command.upgrade) unchanged and reference the existing
variables command, ALEMBIC_CONFIG, total, conn, and DB_PATH to locate the
change.
- Around line 24-36: Move the assertions that check the downgraded state so they
run immediately after calling command.downgrade and before command.upgrade in
test_migration_downgrade_003_removes_substitutes_only; specifically, after
obtaining total and subs from the DB (the variables computed after
command.downgrade), assert total == 11 and subs == 0, then call
command.upgrade(ALEMBIC_CONFIG, "head") to restore state.
- Line 21: The test's DB_PATH computation is brittle because it assumes
DATABASE_URL is an SQLite URL; update the logic around DB_PATH/DATABASE_URL in
tests/test_migrations.py to detect the scheme and only extract a filesystem path
for sqlite URLs (e.g., check that DATABASE_URL starts with "sqlite+" or parse
the scheme), otherwise handle non-sqlite URLs explicitly (raise a clear error,
skip sqlite-specific tests, or use urllib.parse to extract path when scheme ==
"sqlite+aiosqlite"). Locate the DB_PATH assignment and replace the blind replace
with a guarded parse that uses urllib.parse (or an explicit startswith check) so
PostgreSQL or other DB URLs do not produce an invalid path.
---
Nitpick comments:
In `@alembic/env.py`:
- Around line 49-56: Add a one-line docstring to the do_run_migrations function
describing its purpose (e.g., "Run Alembic migrations using the provided DB
connection") to improve readability and satisfy static analysis; place it
immediately after the def do_run_migrations(connection: Connection) -> None:
line so the function now has a concise explanatory docstring.
- Around line 59-67: Add a concise docstring to the async function
run_async_migrations describing its purpose (run Alembic migrations against an
async SQLAlchemy engine built from the alembic config), its behavior (creates
async engine via async_engine_from_config, opens an async connection and calls
do_run_migrations with connection.run_sync, then disposes the engine), and the
return type (None); place it immediately under the def run_async_migrations()
line so readers and tools can understand intent and side effects.
In `@tests/conftest.py`:
- Around line 18-21: The apply_migrations pytest fixture is missing a return
type hint; update the function signature of apply_migrations (the pytest.fixture
that calls command.upgrade with ALEMBIC_CONFIG) to include a -> None return
type. Ensure the annotation is added to the def apply_migrations(...) line and
keep the fixture behavior (calling command.upgrade(ALEMBIC_CONFIG, "head"))
unchanged.
In `@tests/test_migrations.py`:
- Line 24: Add missing return type annotations to the test functions (e.g.,
test_migration_downgrade_003_removes_substitutes_only and the other test
functions at the indicated locations) so they follow project typing guidelines;
update each test function signature to include -> None (e.g., def
test_migration_downgrade_003_removes_substitutes_only() -> None:) and run the
test/type checker to ensure no other typing issues remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6efe6056-7059-4dda-b71b-1f49f5392bc8
📒 Files selected for processing (13)
.github/copilot-instructions.mdCHANGELOG.mdREADME.mdalembic/env.pycodecov.yamldatabases/player_database.pydocs/adr/0002-no-alembic-manual-seed-scripts.mddocs/adr/0009-docker-and-compose-strategy.mddocs/adr/0010-alembic-migration-based-schema-management.mdmain.pyscripts/entrypoint.shtests/conftest.pytests/test_migrations.py
✅ Files skipped from review due to trivial changes (6)
- codecov.yaml
- main.py
- docs/adr/0002-no-alembic-manual-seed-scripts.md
- docs/adr/0010-alembic-migration-based-schema-management.md
- databases/player_database.py
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/entrypoint.sh
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
… hook (#2) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gunicorn.conf.py (1)
21-21: Add parameter typing and mark unused hook arg explicitly.
on_startingis missing a type hint for its parameter, and the arg is intentionally unused.Suggested patch
-def on_starting(server) -> None: +def on_starting(_server: object) -> None: """Apply Alembic migrations once before workers are spawned."""As per coding guidelines,
**/*.py: "Use type hints for function parameters and return values".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gunicorn.conf.py` at line 21, The on_starting hook lacks a parameter type and the argument is intentionally unused; import typing.Any and update the function signature for on_starting to annotate the parameter (e.g., def on_starting(_server: Any) -> None) or keep the name server but annotate it and explicitly mark it unused (e.g., assign to _ or prefix name with underscore) so linters/readers know it's intentionally unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gunicorn.conf.py`:
- Line 21: The on_starting hook lacks a parameter type and the argument is
intentionally unused; import typing.Any and update the function signature for
on_starting to annotate the parameter (e.g., def on_starting(_server: Any) ->
None) or keep the name server but annotate it and explicitly mark it unused
(e.g., assign to _ or prefix name with underscore) so linters/readers know it's
intentionally unused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c831d3bf-27b2-4f28-9191-43724a1ce44a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!**/uv.lock
📒 Files selected for processing (6)
CHANGELOG.mdDockerfilecompose.yamlgunicorn.conf.pypyproject.tomlscripts/entrypoint.sh
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- compose.yaml
- scripts/entrypoint.sh
- CHANGELOG.md
…store (#2) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
nanotaboada
force-pushed
the
feat/2-implement-alembic-database-migrations
branch
from
77a2426 to
c9a01d1
Compare
|
This was
linked to
issues
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
env.pyusesasyncio.run()via thread executor,render_as_batch=Truefor SQLite/PostgreSQL compatibility)001creates theplayerstable,002seeds 11 Starting XI players,003seeds 15 Substitute players — all using deterministic UUID v5 values; each independently reversible by UUIDalembic upgrade headruns automatically at app startup via the lifespan handlerstorage/players-sqlite3.dbremoved from git;*.dbadded to.gitignore;storage/directory deletedCOPY storage/ ./hold/and hold→volume seed logic fromentrypoint.sh; addedCOPY alembic/andalembic.ini;compose.yamlswitches fromSTORAGE_PATHtoDATABASE_URLalembic==1.18.4andasyncpg==0.31.0added to dependenciesTest plan
uv run pytest)uv run pytest --cov=./ --cov-report=term --cov-fail-under=80)uv run alembic upgrade headapplies all three migrations on a fresh DBuv run alembic downgrade -1removes only substitutes; re-upgrade restores themdocker compose upstarts cleanly; migrations applied on first boot; DB persists across restarts via thestoragevolumedocker compose down -vfollowed bydocker compose upre-applies migrations from scratch🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Chores
Documentation
Tests