From 169784f9e12e1fe48cf92edfa6d78bb13e8156ac Mon Sep 17 00:00:00 2001 From: Chet Nichols III Date: Thu, 30 Jul 2026 21:49:15 -0700 Subject: [PATCH] feat(ci): add migration-police for placeholder timestamps Core migrations have historically used `...120000` placeholder timestamps, which means two independently authored PRs can both look fine until their versions collide on `main` -- exactly what #4432 is repairing now. The duplicate-version test can tell us once both migrations are together, but it cannot prevent that collision while the PRs are still independent. So, this adds a separate `migration-police` job that looks only at new files under `crates/api-db/migrations/` and rejects timestamps whose minute and second fields are both `00`. Existing migrations stay accepted, and failures name the migration plus the required `YYYYMMDDhhmmss_description.sql` format in the job log. `STYLE_GUIDE.md` and `AGENTS.md` now point contributors at the same fully populated timestamp rule. This supports https://github.com/NVIDIA/infra-controller/issues/4431 Signed-off-by: Chet Nichols III --- .github/workflows/ci.yaml | 20 ++++++++ AGENTS.md | 5 ++ STYLE_GUIDE.md | 8 +++ scripts/check-migration-filenames.sh | 74 ++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+) create mode 100755 scripts/check-migration-filenames.sh diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 88760ceb9d..bf8cd2e33f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -1362,6 +1362,25 @@ jobs: - name: Run Protolint run: protolint lint -config_path=.protolint.yaml crates/rpc/proto/ + migration-police: + permissions: + contents: read + needs: + - changes + if: ${{ contains(github.ref, 'pull-request/') }} + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + with: + persist-credentials: false + fetch-depth: 0 + + - name: Check new migration timestamps + run: | + git fetch --no-tags origin main:refs/remotes/origin/main + bash scripts/check-migration-filenames.sh --base origin/main + proto-breaking-changes: name: Proto Breaking Changes Check runs-on: ubuntu-latest @@ -2059,6 +2078,7 @@ jobs: - build-release-artifacts-arm-host - security-secret-scan - lint-police + - migration-police - check-rest-core-proto-sync - build-machine-a-tron - build-mat-k8s-controller diff --git a/AGENTS.md b/AGENTS.md index ecf2db1aa0..4c7517f8a5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -169,6 +169,11 @@ verification expectations. See [`STYLE_GUIDE.md`](STYLE_GUIDE.md) for detailed Rust coding conventions. Make sure to review it to ensure changes meet the expected style of the codebase. +Name new Core database migrations with the fully populated +`YYYYMMDDhhmmss_description.sql` format described in +[`STYLE_GUIDE.md`](STYLE_GUIDE.md#database-migrations). The `migration-police` +CI job checks only newly added migrations, so existing filenames remain accepted. + ### Documentation Give every fenced code block a language identifier. Use `bash` or `sh` for diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md index c5ce25834c..a5be6305a9 100644 --- a/STYLE_GUIDE.md +++ b/STYLE_GUIDE.md @@ -342,6 +342,14 @@ your interface `async` just so you can use the tokio Mutex. That way callers can async themselves. Async work should generally be traceable to some I/O or timer that needs to be used, otherwise code should typically be synchronous. +## Database migrations + +Name new Core database migration files with a fully populated 14-digit timestamp: +`YYYYMMDDhhmmss_description.sql`. Use the actual hour, minute, and second values instead of a +trailing `0000` minute-and-second placeholder so independently authored migrations are less likely +to collide. Existing migration filenames remain unchanged, and migrations already on `main` are +immutable. + ## Database transactions Transactions should be used to group write operations together such that they can be rolled back on failure. But do diff --git a/scripts/check-migration-filenames.sh b/scripts/check-migration-filenames.sh new file mode 100755 index 0000000000..ee897e5c4c --- /dev/null +++ b/scripts/check-migration-filenames.sh @@ -0,0 +1,74 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +set -euo pipefail + +usage() { + echo "Usage: check-migration-filenames.sh --base REVISION" + echo " check-migration-filenames.sh MIGRATION_FILE..." +} + +escape_workflow_data() { + local value="$1" + + value="${value//'%'/'%25'}" + value="${value//$'\r'/'%0D'}" + value="${value//$'\n'/'%0A'}" + printf '%s' "${value}" +} + +migration_files=() + +if [[ "${1:-}" == "-h" || "${1:-}" == "--help" ]]; then + usage + exit 0 +elif [[ "${1:-}" == "--base" ]]; then + if (( $# != 2 )); then + usage >&2 + exit 2 + fi + + base_revision="$2" + migration_list="$(mktemp)" + trap 'rm -f "${migration_list}"' EXIT + + git diff --no-renames --diff-filter=A --name-only -z \ + "${base_revision}...HEAD" \ + -- ':(top,glob)crates/api-db/migrations/*.sql' >"${migration_list}" + + while IFS= read -r -d '' migration_file; do + migration_files+=("${migration_file}") + done <"${migration_list}" +elif (( $# > 0 )); then + migration_files=("$@") +else + usage >&2 + exit 2 +fi + +failed=0 + +for migration_file in "${migration_files[@]}"; do + filename="${migration_file##*/}" + timestamp="${filename%%_*}" + + if [[ ! "${filename}" =~ ^[0-9]{14}_.+\.sql$ ]]; then + message="New Core database migration ${migration_file} must use the YYYYMMDDhhmmss_description.sql filename format." + printf '::error title=Invalid migration filename::%s\n' \ + "$(escape_workflow_data "${message}")" + failed=1 + elif [[ "${timestamp}" =~ ^[0-9]{10}0000$ ]]; then + message="New Core database migration ${migration_file} must use a fully populated YYYYMMDDhhmmss timestamp; replace the trailing 0000 minute/second placeholder." + printf '::error title=Placeholder migration timestamp::%s\n' \ + "$(escape_workflow_data "${message}")" + failed=1 + fi +done + +if (( failed )); then + echo "migration-police rejected one or more new database migrations." + exit 1 +fi + +printf 'migration-police checked %d new database migration(s).\n' "${#migration_files[@]}"