mirror of
https://github.com/mattermost/mattermost.git
synced 2026-08-26 21:27:40 -05:00
Add CI check to prevent renumbering or renaming existing DB migrations (#37099)
* Add CI check to prevent renumbering or renaming existing DB migrations Backporting or rebasing a PR can accidentally change the sequence number or description of a migration that already shipped on master, which corrupts the upgrade path for existing installations (see MM-68848). This adds a migration_check tool that compares the working tree's migration files against the base branch and fails when an existing migration's version or name differs. Brand new migrations are ignored. It is wired into the existing "Check migration files" CI job via a new `make check-migration-changes` target. Co-authored-by: mattermost-code <matty-code@mattermost.com> * Apply go fix strings.SplitSeq in migration_check Co-authored-by: mattermost-code <matty-code@mattermost.com> * Retrigger CI after transient Enterprise npm failure Co-authored-by: mattermost-code <matty-code@mattermost.com> * Use github.ref_name as migration check base ref fallback on push Co-authored-by: mattermost-code <matty-code@mattermost.com> * Flag shipped migrations missing from branch by version and name Add reverse comparison so deletions and simultaneous renumber+rename are caught, per CodeRabbit review feedback. Co-authored-by: mattermost-code <matty-code@mattermost.com> * Address PR feedback: 0 answered, 2 resolved, 0 declined * Address PR feedback: 2 answered, 1 resolved, 1 declined * Fix migration check direction and scope to release branches Address isacikgoz review feedback: - Reverse the comparison so it flags migration files the branch HAS that the base branch (origin/master) does NOT, instead of flagging base migrations missing from the branch. A backport branch is an older subset of master, so the old direction reported every newer master migration as an error. - Include the up/down kind in the comparison key so a one-sided rename of only the .up or .down file is no longer masked by its surviving partner. - Restrict the CI step to PRs targeting release-* branches, where the head is expected to be a subset of master; on master-targeted PRs new migrations are normal and would otherwise be flagged as stray. * Scope migration check to PR-added Postgres migrations --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: mattermost-code <matty-code@mattermost.com>
This commit is contained in:
cursor[bot]
co-authored by
Cursor Agent
mattermost-code
Cursor Agent
mattermost-code
parent
f4f69470d3
commit
7fbbf0beef
@@ -213,6 +213,18 @@ jobs:
|
||||
git diff
|
||||
exit 1
|
||||
fi
|
||||
- name: Check for renumbered or renamed migrations
|
||||
# Only backports (PRs targeting a release branch) need this guard: the
|
||||
# migrations they add must keep the exact version+name they have on
|
||||
# master. New migrations on master-targeted PRs are normal and skipped.
|
||||
if: startsWith(github.base_ref, 'release-')
|
||||
run: |
|
||||
git config --global --add safe.directory "$GITHUB_WORKSPACE"
|
||||
git fetch --no-tags --depth=1 origin master "$GITHUB_BASE_REF"
|
||||
make check-migration-changes
|
||||
env:
|
||||
MM_MIGRATION_CHECK_BASE_REF: origin/${{ github.base_ref }}
|
||||
MM_MIGRATION_CHECK_CANONICAL_REF: origin/master
|
||||
check-email-templates:
|
||||
name: Generate email templates
|
||||
needs: go
|
||||
|
||||
+5
-1
@@ -1,4 +1,4 @@
|
||||
.PHONY: build package run stop run-client run-server run-node run-haserver stop-haserver stop-client stop-server restart restart-server restart-client restart-haserver start-docker update-docker clean-dist clean nuke check-style check-client-style check-server-style check-unit-tests test dist run-client-tests setup-run-client-tests cleanup-run-client-tests test-client build-linux build-osx build-freebsd build-windows package-prep package-linux package-osx package-windows internal-test-web-client vet run-server-for-web-client-tests diff-config prepackaged-plugins prepackaged-binaries test-server test-server-ee test-server-elasticsearch test-server-opensearch test-server-quick test-server-race test-mmctl-unit test-mmctl-e2e test-mmctl test-mmctl-coverage mmctl-build mmctl-docs new-migration migrations-extract test-public mocks-public run-server-faketime default-roles-permissions generated
|
||||
.PHONY: build package run stop run-client run-server run-node run-haserver stop-haserver stop-client stop-server restart restart-server restart-client restart-haserver start-docker update-docker clean-dist clean nuke check-style check-client-style check-server-style check-unit-tests test dist run-client-tests setup-run-client-tests cleanup-run-client-tests test-client build-linux build-osx build-freebsd build-windows package-prep package-linux package-osx package-windows internal-test-web-client vet run-server-for-web-client-tests diff-config prepackaged-plugins prepackaged-binaries test-server test-server-ee test-server-elasticsearch test-server-opensearch test-server-quick test-server-race test-mmctl-unit test-mmctl-e2e test-mmctl test-mmctl-coverage mmctl-build mmctl-docs new-migration migrations-extract check-migration-changes test-public mocks-public run-server-faketime default-roles-permissions generated
|
||||
|
||||
ROOT := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))
|
||||
|
||||
@@ -929,5 +929,9 @@ migrations-extract:
|
||||
@echo "# Autogenerated file to synchronize migrations sequence in the PR workflow, please do not edit." > channels/db/migrations/migrations.list
|
||||
find channels/db/migrations -maxdepth 2 -mindepth 2 | sort >> channels/db/migrations/migrations.list
|
||||
|
||||
check-migration-changes: ## Fails if a Postgres migration added on top of the base branch was renumbered or renamed relative to master.
|
||||
@echo "Checking for renumbered or renamed migrations"
|
||||
./scripts/check_migration_changes.sh
|
||||
|
||||
test-local-filestore: setup-go-work # Run tests for local filestore
|
||||
$(GO) test ./platform/shared/filestore -run '^TestLocalFileBackend' -v
|
||||
|
||||
Executable
+78
@@ -0,0 +1,78 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# Backport safety check (Postgres only).
|
||||
#
|
||||
# A release branch is an older subset of master. When a PR backports a migration
|
||||
# it must keep the exact version number and name that migration already has on
|
||||
# master; renumbering or renaming a shipped migration breaks the upgrade path
|
||||
# (the MM-68848 class of bug).
|
||||
#
|
||||
# We therefore only look at the migrations this PR ADDS on top of its base branch
|
||||
# and require each of them to already exist on master. Files that are already on
|
||||
# the base branch are left untouched, so historical states such as the Postgres
|
||||
# migrations renamed before the pre-migration infra landed do not produce false
|
||||
# positives.
|
||||
#
|
||||
# MySQL is intentionally skipped: support was dropped after v11.0, so older
|
||||
# release branches still carry MySQL migrations that no longer exist on master.
|
||||
#
|
||||
# Portable across the bash 3.2 that ships with macOS and Linux CI: uses only
|
||||
# POSIX-ish tools plus process substitution, no bash 4 features.
|
||||
#
|
||||
set -euo pipefail
|
||||
export LC_ALL=C
|
||||
|
||||
# Branch the PR targets. The migrations the PR adds are everything HEAD has that
|
||||
# this ref does not.
|
||||
base_ref="${MM_MIGRATION_CHECK_BASE_REF:-origin/master}"
|
||||
# Canonical source of shipped migrations the additions must match.
|
||||
canonical_ref="${MM_MIGRATION_CHECK_CANONICAL_REF:-origin/master}"
|
||||
|
||||
repo_root="$(git rev-parse --show-toplevel)"
|
||||
# Postgres only; MySQL migrations linger on old release branches and are skipped.
|
||||
migrations_dir="server/channels/db/migrations/postgres"
|
||||
|
||||
for ref in "$base_ref" "$canonical_ref"; do
|
||||
if ! git -C "$repo_root" rev-parse --verify --quiet "${ref}^{commit}" >/dev/null; then
|
||||
echo "Ref '$ref' not found. Fetch it first (e.g. 'git fetch origin master')" >&2
|
||||
echo "or set MM_MIGRATION_CHECK_BASE_REF / MM_MIGRATION_CHECK_CANONICAL_REF." >&2
|
||||
exit 2
|
||||
fi
|
||||
done
|
||||
|
||||
# path -> "version_name.kind"; the up/down kind is part of the identity so a
|
||||
# one-sided rename can't be masked by its surviving partner file.
|
||||
norm='s#^'"$migrations_dir"'/([0-9]+_.+)\.(up|down)\.sql$#\1.\2#p'
|
||||
|
||||
ref_files() {
|
||||
git -C "$repo_root" ls-tree -r --name-only "$1" -- "$migrations_dir" \
|
||||
| sed -nE "$norm" | sort -u
|
||||
}
|
||||
|
||||
head_files() {
|
||||
# Tracked + untracked files actually present on disk, so a plain `mv`
|
||||
# without `git add` is still caught.
|
||||
git -C "$repo_root" ls-files --cached --others --exclude-standard -- "$migrations_dir" \
|
||||
| while IFS= read -r path; do
|
||||
[ -f "$repo_root/$path" ] && printf '%s\n' "$path"
|
||||
done \
|
||||
| sed -nE "$norm" | sort -u
|
||||
}
|
||||
|
||||
# Migrations this PR adds on top of its base branch.
|
||||
added="$(comm -23 <(head_files) <(ref_files "$base_ref"))"
|
||||
|
||||
# Of those, the ones that don't exist on the canonical branch are renames,
|
||||
# renumbers, or otherwise stray.
|
||||
stray="$(comm -23 <(printf '%s\n' "$added" | sed '/^$/d') <(ref_files "$canonical_ref"))"
|
||||
|
||||
if [ -n "$stray" ]; then
|
||||
count="$(printf '%s\n' "$stray" | wc -l | tr -d '[:space:]')"
|
||||
echo "Found $count migration file(s) added by this branch that do not exist on $canonical_ref:" >&2
|
||||
printf '%s\n' "$stray" | while IFS= read -r m; do
|
||||
echo " - ${m}.sql is added on this branch but not present on $canonical_ref; renaming or renumbering a shipped migration breaks upgrades. Add a new migration instead." >&2
|
||||
done
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "All migrations added by this branch exist on $canonical_ref."
|
||||
Reference in New Issue
Block a user