feat: add check-migration-order action, wired into pull-request-kotlin [CPONETOPS-1361] - #343
Merged
Merged
Conversation
Runs it from pull-request-kotlin so every Kotlin repo on the shared workflow gets it; the job self-skips when src/main/resources/db/migration is absent. Each migration-paths line is an independent Flyway version namespace, so multi-module repos with separate databases are not pooled.
Discovers every "migration" directory under src/main/resources holding a .sql, unioned from the working tree and the base branch, so ClickHouse roots, nested modules and resources/migration are covered without per-repo config. The root is the migration directory itself, not each directory containing a .sql -- Flyway pools common/, <env>/ and stored/procedures/ into one version namespace, so splitting them would let an out-of-order migration pass.
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
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.
Description
Adds a shared
check-migration-ordercomposite action and runs it frompull-request-kotlin.yml, so every Kotlin repo on the shared workflow gets the check with no per-repo configuration.It fails a PR that adds a Flyway migration sorting before one already applied on the base branch, misnames a new migration, or edits/renames/deletes a committed one.
What's in this PR
1. The action —
.github/actions/check-migration-order/(action.yml,check-migration-order.sh,README.md).2. The wiring — a
check-migration-orderjob inpull-request-kotlin.yml:No
with:block — the action discovers its own roots. It's an independent job (noneeds: setup) onlinux-arm64so it runs in parallel and finishes in seconds rather than waiting on runner resolution. Callers get it automatically; nothing to add per repo.Why a composite action rather than a reusable workflow
A reusable workflow's status check is named
<caller job name> / <inner job name>— this repo's ownpull-request-kotlin.ymlproves it: itsSetup/Lint/Test with code coveragejobs surface asPull Request / Setupand so on. Routing service-ocpi through a reusable workflow would therefore rename its required checkCheck Migration File Namingand force a branch-protection change. A composite action leaves the caller's job name untouched.Layout follows the existing
argocd-wait-sync/pr-title-checkconvention:action.yml+README.md+ a separate.shinvoked via"${{ github.action_path }}/...".Why the check exists
Flyway runs without
outOfOrder, so a version sorting before an already-applied one fails validation and kills the deploy's migration step. With an ArgoCDPreSynchook the sync never completes and the environment silently stays on the old image.Not hypothetical: service-charges #878 added
V2026.09.02.10.00and merged after #892 addedV2026.09.04.10.00. Staging applied #892 first,09.02became pending behind it, and four consecutive deploys failed while staging sat on a stale image for a day.The load-bearing detail is comparing against the base branch tip, not
git merge-base. The merge base cannot contain migrations that landed after the branch was cut, so a merge-base comparison — which is what service-ocpi's existing check does — cannot catch that case at all.Auto-discovery, and the trap it avoids
migration-pathsdefaults to empty, meaning discover: every directory namedmigrationundersrc/main/resourcesholding a.sqlsomewhere beneath it, unioned from the working tree and the base branch.The root is the
migrationdirectory itself, not every directory that contains a.sql. That distinction is the whole correctness argument. Flyway pools a root's subdirectories (common/,<env>/,stored/procedures/) into one version namespace. service-charges hasdb/migration/commonat2026.09.09anddb/migration/stored/proceduresat2021.10.27; if those were treated as separate roots, a newstored/proceduresmigration dated 2022 would pass (2022 > 2021) while Flyway rejects it out-of-order against the pooled 2026 ceiling. A false pass is exactly the bug this check exists to prevent, so discovery stops at themigrationdirectory.Conversely, genuinely separate databases must not be pooled. In service-ocpp the processor's newest migration is from 2026 and the gateway's from 2022, and I confirmed gateway depends on
:common/:gateway-core, not:processor-common— separate classpaths, separate Flyway instances. A single repo-wide root wrongly rejects a valid new gateway migration.QA
Swept every Kotlin checkout. Discovery finds the right number of independent namespaces in each, all green:
src/main/resources/db/migrationapp/…)db/+clickhouse/), wallet (persistence/db/+testing/clickhouse/)resources/migration, nodb/Failure detection verified by replaying the real service-charges incident, four diagnostics in one run — note the fourth, which is the pooling case above:
That sweep earned its keep — it caught two bugs in the discovery code before this was pushed: the base-branch derivation required a directory between
resources/andmigration/(so it missed data-fusion'sresources/migrationentirely), andgrepexiting 1 on no-match aborted the whole script underset -e+pipefail, turning "no migrations here" into a hard failure. Both fixed.shellcheck -x -e SC1091clean,actionlintclean on the modified workflow, both YAML files parse andaction.ymlvalidates as a composite action.Three things the action does that the per-repo copies didn't:
db/layouts a static default missed.git merge-baseon a shallow clone silently returns the wrong answer rather than failing — too sharp an edge to leave to each caller rememberingfetch-depth: 0.Context
CPONETOPS-1361
Follow-ups once this merges — actions here are consumed at
@main, so it takes effect for every caller immediately:Check Migration File Naming, so its required check is unaffected, and ocpp no longer needs its hardcoded two-root loop.Known limitation:
pull_requestdoesn't re-fire when the base branch moves, so two PRs opened in parallel can still both be green. Closing that needs Require branches to be up to date before merging; without it this is a strong nudge, not a guarantee.Scope: Flyway-style versioned filenames. Version extraction (strip leading
B/V, strip__description) is Flyway-specific — Laravel- or Prisma-style migrations would need a second extraction mode.