CI: check that new migrations are numbered above main's highest - #476
Merged
alex-clickhouse merged 2 commits intoOct 1, 2026
Merged
Conversation
run_migrations() reads MAX(version) once and skips every migration at or below it, so a migration merged with a number at or below the highest one already on main never runs on a database migrated to main; only fresh databases get it. Parallel PRs each pick the next free number against the main they forked from, so the second to merge can land below the first (ClickHouse#462 was first written as v045 while main was at v049). scripts/check_migration_numbers.py lists the base's migrations with git ls-tree -z (NUL-delimited, so git does not quote unusual names) and fails if a migration file present in the working tree but not on the base is numbered at or below the base's highest version. It uses the same vN_*.py rule as discover_migrations(), pinned by a test on the real directory. The new step runs last in backend-tests, on pull_request only, with !cancelled() so a test failure does not hide it. It fetches the base branch at run time instead of reading the merge commit's first parent, so re-running a PR's CI compares against today's main, not the main the PR was opened against. docs/architecture.md and the nerve-dev skill template still told contributors to name their migration v017 / v018. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
That sounds like a good idea. |
A PR's check can go green before another migration merges, and PR workflows do not re-run when main moves. On a push to main the step now compares the pushed tree with github.event.before, the commit the push replaced, so a migration merged out of order turns that commit red. Push runs no longer share a concurrency group: 33 of the last 133 push runs on main were cancelled by the next push, and a cancelled run would let the out-of-order merge through, because the next push's base already contains it. A push run gets its own group (the run id), so a manual dispatch on main cannot cancel it either. PRs and other branches still cancel superseded runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Done in b8831c9: on a push to |
alex-clickhouse
approved these changes
Oct 1, 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
2 participants
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.
Follow-up to #462 (item 2 of #462 (comment)).
run_migrations()skips every migration at or belowMAX(version), so a migration merged with a number at or below main's highest never runs on an already-migrated database. #462 was first written asv045while main was atv049; a database atv049upgraded with it stayed atschema_version=49without the new column.The change
scripts/check_migration_numbers.py BASE: fails if a migration file on the branch but not onBASEis numbered at or belowBASE's highest. SamevN_*.pyrule asdiscover_migrations(), pinned by a test.backend-tests. On a PR it fetchesmainwhen it runs, so re-running a PR's CI compares against today's main, not the one the PR forked from.mainthe same step compares againstgithub.event.before, so a migration merged out of order turns that commit red (a PR's check can go green before another migration merges, and PR checks do not re-run when main moves). Push runs now get their own concurrency group and are never cancelled: 33 of the last 133 onmainwere cancelled by the next push, which would let such a merge through.docs/architecture.mdand thenerve-devskill template no longer suggestv017/v018as the number to use.Validation
On real trees, against
main: #462 atv045fails, #404/#422/#462 pass; with #462 as the base, #404 (v050) and #422 (v050,v051) fail. 10 tests, including a CLI run against a git base with a non-ASCII migration name, each broken by a mutation of the check. Full suite 3911 passed vs 3901 onmain.Push arm, run from a shallow clone against
ClickHouse/nerve:github.event.beforefetches by SHA; addingv045orv049over main'sv049fails,v050passes.actionlint(with shellcheck) is clean.🤖 Generated with Claude Code