fix(ci): enforce a single Alembic migration head - #42890
Conversation
The existing DB-migration-conflict check only posts an advisory comment on other open PRs when a new migration lands on the base branch; it never actually computes whether the merge leaves more than one Alembic head. Two independent migrations that both branch off the same parent can merge without warning and silently fork the history until someone notices flask db upgrade is broken (see #42878, which needed a manual merge revision after exactly this happened). Add a job that runs `superset db heads` against the PR's merge ref (and against master/release branches on push, as a tripwire) and fails if the count isn't exactly 1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dosubot
Bot
added
the
github_actions
|
Bito Automatic Review Skipped - Files Excluded |
Marking the previous job a required status check while it only ran on `paths: superset/migrations/**` would have blocked every PR that doesn't touch migrations forever, since a required check that never reports can never be satisfied. Move it to its own workflow with no path filter (matching dependency-review.yml's shape) and decide whether migrations changed inside the job instead, via the PR's changed-files API, so it always reports a status. Also register it in .asf.yaml's required_status_checks so ASF Infra actually enforces it on master.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #88c6a6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
The existing
Check DB migration conflictworkflow is advisory-only: when amigration lands on the base branch, it scans other open PRs that also
touch
superset/migrations/**and posts a warning comment asking them torebase. It never actually computes whether the merge leaves more than one
Alembic head, and nothing about it blocks a merge.
That gap is exactly how master ended up with two divergent heads
(
4f145192b583andc4a1b8e2d739) this week — two unrelated migrationsbranched off the same parent and merged independently, and nothing caught
it until
flask db upgradebroke and #42878 had to land a manual mergerevision to fix it retroactively.
This adds a new workflow,
enforce-single-migration-head.yml, that runssuperset db headsand fails if the count isn't exactly 1:pull_requestit checks out the default merge ref (base + this PRcombined, which is what
actions/checkoutgives you automatically forpull_request-triggered runs).flask-migrate'sheadscommand onlywalks the migration script directory — it never touches a real database —
so this doesn't need Postgres/Redis services, just a syntactically valid
SQLALCHEMY_DATABASE_URIto let the app instantiate.pushtomaster/release branches (so if a required-checkbypass ever lets a fork through, master itself fails loudly and
immediately instead of drifting until someone notices a broken upgrade).
paths:filter on the trigger. This is registered asa required status check below, and a required check that never runs for
a given PR blocks that PR from merging forever — GitHub has no way to
mark an "Expected" check satisfied if it never reports. So the workflow
fires on every PR, and a first step asks the GitHub API whether this PR's
files touch
superset/migrations/to decide whether to actually do the(cheap, but non-zero) work, while still always reporting a status. Matches
the pattern
dependency-review.ymlalready uses for the same reason.enforce-single-migration-headin.asf.yaml'srequired_status_checks.contexts, so ASF Infra actually enforces it onmasterinstead of it just being a check nobody's required to look at.The existing advisory-comment job is left as-is; it's still useful context
for the "hey, someone else also touched migrations" case, just no longer
the only line of defense.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — CI workflow only.
TESTING INSTRUCTIONS
zizmor(GHA security audit, run via pre-commit) passes clean on bothchanged workflow files.
yaml.safe_loadfor the workflow and.asf.yaml.flask_migrate/alembicinternals:ScriptDirectory.from_config(...).get_heads()(what theheadscommandcalls) parses migration files directly and never invokes
env.py, so itneeds no DB connectivity — consistent with the existing
superset db upgradeinvocation pattern already used insuperset-app-cli.yml, justwithout that job's Postgres/Redis services.
lint-check,dependency-review,frontend-build) that.asf.yamlcontexts match thejob id, not a job-level
name:override, so this job intentionally hasno
name:field to keep the two in sync.that branch off the same parent and confirm the second one to update
fails this check.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code