forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 12 Pull requests 3 Projects Releases Packages Wiki Activity Actions

fix: merge forked alembic migration chain #169

Merged
forgejo_admin merged 2 commits from 166-fix-alembic-forked-migration-chain into main 2026-03-26 03:47:05 +00:00
Conversation 3 Commits 2 Files changed 1 +26
forgejo_admin commented 2026-03-25 13:38:07 +00:00
Owner
Copy link

Summary

  • Migrations 019 and 020 both declared down_revision = "018", creating a forked Alembic migration chain with two heads on main
  • Adds a merge migration (021) that unifies the two heads into a single linear chain
  • No existing migration files modified -- they are already applied in prod

Changes

  • alembic/versions/021_merge_heads.py: new merge migration with down_revision = ("019", "020") and empty upgrade/downgrade functions

Test Plan

  • alembic heads returns exactly one head (021) after this change
  • alembic history shows clean merge: 019, 020 -> 021 (head) (mergepoint)
  • 521/529 tests pass; 8 failures are pre-existing on main (Keycloak DNS, test_temp_team_dedup, jersey announcement tests)
  • Safe to apply in prod: alembic upgrade head will stamp 021 as current with no schema changes

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related

## Summary - Migrations 019 and 020 both declared `down_revision = "018"`, creating a forked Alembic migration chain with two heads on main - Adds a merge migration (021) that unifies the two heads into a single linear chain - No existing migration files modified -- they are already applied in prod ## Changes - `alembic/versions/021_merge_heads.py`: new merge migration with `down_revision = ("019", "020")` and empty upgrade/downgrade functions ## Test Plan - [x] `alembic heads` returns exactly one head (`021`) after this change - [x] `alembic history` shows clean merge: `019, 020 -> 021 (head) (mergepoint)` - [x] 521/529 tests pass; 8 failures are pre-existing on main (Keycloak DNS, test_temp_team_dedup, jersey announcement tests) - [x] Safe to apply in prod: `alembic upgrade head` will stamp 021 as current with no schema changes ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/basketball-api#166
fix: merge forked alembic migration chain (019/020 both descended from 018)
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
d3deabdfd4 
Migrations 019_player_teams_junction and 020_add_custom_notes_to_player
were both merged to main with down_revision = "018", creating two heads.
This adds a merge migration (021) that unifies them into a single chain
without modifying any existing migration files already applied in prod.

Closes #166

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-25 13:38:23 +00:00
Author
Owner
Copy link

Review: LGTM

Diff reviewed: 1 file, +26 lines, 0 deletions

Findings: None. This is a textbook Alembic merge migration:

  • down_revision = ("019", "020") correctly uses a tuple to merge the two heads
  • Empty upgrade/downgrade -- no schema changes, just chain repair
  • No existing migration files modified (safe for prod where 019/020 are already applied)
  • alembic heads confirmed single head (021) after the change
  • Docstring clearly explains the problem and fix

No issues found.

## Review: LGTM **Diff reviewed**: 1 file, +26 lines, 0 deletions **Findings**: None. This is a textbook Alembic merge migration: - `down_revision = ("019", "020")` correctly uses a tuple to merge the two heads - Empty upgrade/downgrade -- no schema changes, just chain repair - No existing migration files modified (safe for prod where 019/020 are already applied) - `alembic heads` confirmed single head (021) after the change - Docstring clearly explains the problem and fix **No issues found.**
forgejo_admin commented 2026-03-26 03:40:15 +00:00
Author
Owner
Copy link

PR #169 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic

This PR adds a single file: alembic/versions/021_merge_heads.py -- a merge migration that reunifies two Alembic heads.

Root cause verified. Migrations 019 (019_player_teams_junction.py) and 020 (020_add_custom_notes_to_player.py) both declare down_revision = "018", creating a forked Alembic chain with two heads. This is a real bug -- alembic upgrade head would fail with "Multiple head revisions" on a fresh database.

Merge migration correctness:

  • revision = "021" -- follows the project's numeric naming convention (001-020 all use numeric IDs, with the exception of the legacy e09c9e678004 at position 006)
  • down_revision = ("019", "020") -- correct Alembic tuple syntax for merge migrations
  • branch_labels = None and depends_on = None -- correct, no branch labeling needed
  • upgrade() and downgrade() are both pass -- correct, this is a merge-only migration with no schema changes
  • Docstring accurately describes the problem and references both parent revisions
  • from alembic import op and import sqlalchemy as sa with # noqa: F401 -- these imports are unused but follow the project's migration template convention. Acceptable.

No existing migrations modified -- the original 019 and 020 files remain untouched. Since both are already applied in production, modifying them would be dangerous. The merge-forward approach is the correct pattern.

env.py note (pre-existing, out of scope): alembic/env.py imports Coach, Outbox, Parent, Player, Registration, Tenant but does not import Team. This means alembic revision --autogenerate may not detect changes to the Team model. This predates this PR and is not a blocker here.

BLOCKERS

None.

This PR introduces no new functionality -- it is a structural fix to Alembic's migration chain. The BLOCKER criteria do not apply:

  • No new functionality = no test coverage requirement (a merge migration with empty upgrade/downgrade has nothing to test beyond alembic heads returning one head, which was verified manually per the test plan)
  • No user input = no validation concerns
  • No secrets = no credential exposure
  • No auth logic = no DRY risk

NITS

  1. Create Date format: The docstring uses Create Date: 2026-03-25 while earlier migrations use the full datetime format (2026-03-10 18:27:23.147242). Cosmetic inconsistency only -- the project actually uses both formats across the migration history, so this is fine as-is.

SOP COMPLIANCE

  • Branch named after issue: 166-fix-alembic-forked-migration-chain references issue #166
  • PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
  • Related references parent issue: "Closes forgejo_admin/basketball-api#166"
  • No secrets committed
  • No unnecessary file changes: 1 file, 26 lines -- tightly scoped
  • Commit messages are descriptive

Missing from SOP: The PR body does not include a plan slug reference in the Related section. However, this is a bugfix (not plan-driven work), so a plan slug is not required.

PROCESS OBSERVATIONS

Deployment frequency impact: Positive. This unblocks alembic upgrade head on fresh databases. Without this fix, any new environment provisioning or CI migration test would fail.

Change failure risk: Very low. The migration performs no schema changes. Applying it in production will simply stamp revision 021 as current.

Pre-existing test failures (discovered scope): The PR reports 8 of 529 tests failing on main. Based on the test plan and codebase analysis, these appear to be:

  • Keycloak DNS failures: Tests in tests/test_keycloak_integration.py that depend on Keycloak connectivity (test env uses http://localhost:8080/realms/test -- failures indicate mock gaps or DNS resolution issues in CI)
  • test_temp_team_dedup in tests/test_admin_teams.py (line 290): Tests the many-to-many team assignment dedup logic introduced in PR #164 / migration 019. Likely a fixture or model relationship issue from the junction table migration
  • Jersey announcement tests in tests/test_jersey.py: test_sends_announcement_to_all_parents (line 953) and test_announcement_email_has_no_password_field (line 1148) -- likely related to email service mock configuration or the password-removal cleanup (#168)

These 8 failures predate this PR and should be tracked as separate issues. Recommend creating a Forgejo issue for the pre-existing test failures to prevent technical debt accumulation.

Also noted (out of scope): Team model is not imported in alembic/env.py, which means autogenerate would miss Team table changes. Worth a separate fix.

VERDICT: APPROVED

## PR #169 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic This PR adds a single file: `alembic/versions/021_merge_heads.py` -- a merge migration that reunifies two Alembic heads. **Root cause verified.** Migrations 019 (`019_player_teams_junction.py`) and 020 (`020_add_custom_notes_to_player.py`) both declare `down_revision = "018"`, creating a forked Alembic chain with two heads. This is a real bug -- `alembic upgrade head` would fail with "Multiple head revisions" on a fresh database. **Merge migration correctness:** - `revision = "021"` -- follows the project's numeric naming convention (001-020 all use numeric IDs, with the exception of the legacy `e09c9e678004` at position 006) - `down_revision = ("019", "020")` -- correct Alembic tuple syntax for merge migrations - `branch_labels = None` and `depends_on = None` -- correct, no branch labeling needed - `upgrade()` and `downgrade()` are both `pass` -- correct, this is a merge-only migration with no schema changes - Docstring accurately describes the problem and references both parent revisions - `from alembic import op` and `import sqlalchemy as sa` with `# noqa: F401` -- these imports are unused but follow the project's migration template convention. Acceptable. **No existing migrations modified** -- the original 019 and 020 files remain untouched. Since both are already applied in production, modifying them would be dangerous. The merge-forward approach is the correct pattern. **env.py note (pre-existing, out of scope):** `alembic/env.py` imports `Coach, Outbox, Parent, Player, Registration, Tenant` but does not import `Team`. This means `alembic revision --autogenerate` may not detect changes to the Team model. This predates this PR and is not a blocker here. ### BLOCKERS None. This PR introduces no new functionality -- it is a structural fix to Alembic's migration chain. The BLOCKER criteria do not apply: - **No new functionality** = no test coverage requirement (a merge migration with empty upgrade/downgrade has nothing to test beyond `alembic heads` returning one head, which was verified manually per the test plan) - **No user input** = no validation concerns - **No secrets** = no credential exposure - **No auth logic** = no DRY risk ### NITS 1. **Create Date format**: The docstring uses `Create Date: 2026-03-25` while earlier migrations use the full datetime format (`2026-03-10 18:27:23.147242`). Cosmetic inconsistency only -- the project actually uses both formats across the migration history, so this is fine as-is. ### SOP COMPLIANCE - [x] Branch named after issue: `166-fix-alembic-forked-migration-chain` references issue #166 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references parent issue: "Closes forgejo_admin/basketball-api#166" - [x] No secrets committed - [x] No unnecessary file changes: 1 file, 26 lines -- tightly scoped - [x] Commit messages are descriptive **Missing from SOP:** The PR body does not include a plan slug reference in the Related section. However, this is a bugfix (not plan-driven work), so a plan slug is not required. ### PROCESS OBSERVATIONS **Deployment frequency impact**: Positive. This unblocks `alembic upgrade head` on fresh databases. Without this fix, any new environment provisioning or CI migration test would fail. **Change failure risk**: Very low. The migration performs no schema changes. Applying it in production will simply stamp revision 021 as current. **Pre-existing test failures (discovered scope):** The PR reports 8 of 529 tests failing on main. Based on the test plan and codebase analysis, these appear to be: - **Keycloak DNS failures**: Tests in `tests/test_keycloak_integration.py` that depend on Keycloak connectivity (test env uses `http://localhost:8080/realms/test` -- failures indicate mock gaps or DNS resolution issues in CI) - **`test_temp_team_dedup`** in `tests/test_admin_teams.py` (line 290): Tests the many-to-many team assignment dedup logic introduced in PR #164 / migration 019. Likely a fixture or model relationship issue from the junction table migration - **Jersey announcement tests** in `tests/test_jersey.py`: `test_sends_announcement_to_all_parents` (line 953) and `test_announcement_email_has_no_password_field` (line 1148) -- likely related to email service mock configuration or the password-removal cleanup (#168) These 8 failures predate this PR and should be tracked as separate issues. Recommend creating a Forgejo issue for the pre-existing test failures to prevent technical debt accumulation. **Also noted (out of scope):** `Team` model is not imported in `alembic/env.py`, which means autogenerate would miss Team table changes. Worth a separate fix. ### VERDICT: APPROVED
forgejo_admin force-pushed 166-fix-alembic-forked-migration-chain from d3deabdfd4
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
to b983cf5784
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
2026-03-26 03:43:56 +00:00 Compare
forgejo_admin commented 2026-03-26 03:45:35 +00:00
Author
Owner
Copy link

PR #169 Review (Re-review after rebase)

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic

Migration chain verification -- I traced every down_revision in alembic/versions/ to verify the DAG:

... -> 017 -> 018 -> 019 -----------\
                  \                  --> 022 (merge, single head)
                   -> 020 -> 021 --/
  • 018: down_revision = "017" -- linear, correct
  • 019: down_revision = "018" -- fork arm 1, correct
  • 020: down_revision = "018" -- fork arm 2, correct
  • 021: down_revision = "020" -- extends arm 2 (added by PR #163), correct
  • 022: down_revision = ("019", "021") -- merges the two leaf heads, correct

The merge migration correctly identifies 019 and 021 as the two heads after the rebase incorporated migration 021 from PR #163. The tuple ("019", "021") will cause alembic heads to return exactly one head: 022.

Merge migration content: upgrade() and downgrade() are both pass, which is correct -- merge migrations carry no schema changes. The # noqa: F401 comments on the unused imports are appropriate since alembic/versions is excluded from ruff in pyproject.toml (line 37), but the noqa annotations are still good practice.

Docstring accuracy: The docstring in 022_merge_heads.py correctly describes the fork topology: "019 descends from 018 directly, 021 descends from 018 via 020."

BLOCKERS

None. This is a zero-schema-change merge migration. No new functionality requiring tests. No user input. No secrets. No auth logic.

NITS

  1. Stale PR body: The PR description still references the pre-rebase numbering throughout:

    • Says "merge migration (021)" -- should be 022
    • Says alembic/versions/021_merge_heads.py -- should be 022_merge_heads.py
    • Says down_revision = ("019", "020") -- should be ("019", "021")
    • Says alembic heads returns 021 -- should be 022
    • Says 019, 020 -> 021 (head) -- should be 019, 021 -> 022 (head)

    The code is correct; only the PR body text is stale from before the rebase. Recommend updating the PR body to match the actual file for future reference.

SOP COMPLIANCE

  • Branch named after issue: 166-fix-alembic-forked-migration-chain references issue #166
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related section references parent issue: "Closes forgejo_admin/basketball-api#166"
  • No secrets committed
  • No unnecessary file changes -- single file, 26 additions, 0 deletions
  • Commit messages are descriptive
  • PR body accuracy: PR description text is stale (pre-rebase numbering) -- non-blocking but should be updated

PROCESS OBSERVATIONS

Clean, minimal fix. One file, no schema changes, no risk of change failure. The fork was caused by two PRs (#164 and #165) landing against the same parent migration 018 without coordinating revision numbers, then PR #163 extending one arm. This merge migration is the standard Alembic pattern to resolve multi-head situations. Safe for alembic upgrade head in prod -- it will simply stamp 022 as current.

VERDICT: APPROVED

## PR #169 Review (Re-review after rebase) ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic **Migration chain verification** -- I traced every `down_revision` in `alembic/versions/` to verify the DAG: ``` ... -> 017 -> 018 -> 019 -----------\ \ --> 022 (merge, single head) -> 020 -> 021 --/ ``` - `018`: `down_revision = "017"` -- linear, correct - `019`: `down_revision = "018"` -- fork arm 1, correct - `020`: `down_revision = "018"` -- fork arm 2, correct - `021`: `down_revision = "020"` -- extends arm 2 (added by PR #163), correct - `022`: `down_revision = ("019", "021")` -- merges the two leaf heads, correct The merge migration correctly identifies **019** and **021** as the two heads after the rebase incorporated migration 021 from PR #163. The tuple `("019", "021")` will cause `alembic heads` to return exactly one head: `022`. **Merge migration content**: `upgrade()` and `downgrade()` are both `pass`, which is correct -- merge migrations carry no schema changes. The `# noqa: F401` comments on the unused imports are appropriate since `alembic/versions` is excluded from ruff in `pyproject.toml` (line 37), but the noqa annotations are still good practice. **Docstring accuracy**: The docstring in `022_merge_heads.py` correctly describes the fork topology: "019 descends from 018 directly, 021 descends from 018 via 020." ### BLOCKERS None. This is a zero-schema-change merge migration. No new functionality requiring tests. No user input. No secrets. No auth logic. ### NITS 1. **Stale PR body**: The PR description still references the pre-rebase numbering throughout: - Says "merge migration (021)" -- should be 022 - Says `alembic/versions/021_merge_heads.py` -- should be `022_merge_heads.py` - Says `down_revision = ("019", "020")` -- should be `("019", "021")` - Says `alembic heads` returns `021` -- should be `022` - Says `019, 020 -> 021 (head)` -- should be `019, 021 -> 022 (head)` The **code** is correct; only the PR body text is stale from before the rebase. Recommend updating the PR body to match the actual file for future reference. ### SOP COMPLIANCE - [x] Branch named after issue: `166-fix-alembic-forked-migration-chain` references issue #166 - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related section references parent issue: "Closes forgejo_admin/basketball-api#166" - [x] No secrets committed - [x] No unnecessary file changes -- single file, 26 additions, 0 deletions - [x] Commit messages are descriptive - [ ] PR body accuracy: PR description text is stale (pre-rebase numbering) -- non-blocking but should be updated ### PROCESS OBSERVATIONS Clean, minimal fix. One file, no schema changes, no risk of change failure. The fork was caused by two PRs (#164 and #165) landing against the same parent migration 018 without coordinating revision numbers, then PR #163 extending one arm. This merge migration is the standard Alembic pattern to resolve multi-head situations. Safe for `alembic upgrade head` in prod -- it will simply stamp 022 as current. ### VERDICT: APPROVED
forgejo_admin deleted branch 166-fix-alembic-forked-migration-chain 2026-03-26 03:47:05 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/basketball-api!169
No description provided.