Fix duplicate migration versions
Authored by
mfwolffe <wolffemf@dukes.jmu.edu>
- SHA
970ad46788501ba6414ee149f0404134a2bdbbcd- Parents
-
24ab250 - Tree
667ddfd
tenseleyflow/shithub
/
970ad46
Browse files
9 changed files
| Status | File | + | - |
|---|---|---|---|
| M |
.github/workflows/ci.yml
|
3 | 0 |
| M |
Makefile
|
5 | 2 |
| M |
docs/internal/actions-schema.md
|
4 | 2 |
| M |
docs/internal/db.md
|
3 | 1 |
| M |
docs/internal/runbooks/actions-runner.md
|
1 | 1 |
| R |
internal/migrationsfs/migrations/0052_runner_jwt_used.sql →
internal/migrationsfs/migrations/0053_runner_jwt_used.sql
|
0 | 0 |
| R |
internal/migrationsfs/migrations/0052_push_events_web_protocol.sql →
internal/migrationsfs/migrations/0054_push_events_web_protocol.sql
|
0 | 0 |
| A |
internal/migrationsfs/migrationsfs_test.go
|
54 | 0 |
| A |
scripts/lint-migration-versions.sh
|
29 | 0 |
.github/workflows/ci.ymlmodified@@ -42,6 +42,9 @@ jobs: | |||
| 42 | # accumulated during S00–S40 refactors (audit 2026-05-10 M1). | 42 | # accumulated during S00–S40 refactors (audit 2026-05-10 M1). |
| 43 | run: scripts/lint-unused.sh | 43 | run: scripts/lint-unused.sh |
| 44 | 44 | ||
| 45 | + - name: Lint - migration versions | ||
| 46 | + run: scripts/lint-migration-versions.sh | ||
| 47 | + | ||
| 45 | - name: Test | 48 | - name: Test |
| 46 | run: go test -trimpath ./... | 49 | run: go test -trimpath ./... |
| 47 | 50 | ||
Makefilemodified@@ -2,7 +2,7 @@ | |||
| 2 | # Targets mirror what CI runs. The Makefile is the source of truth. | 2 | # Targets mirror what CI runs. The Makefile is the source of truth. |
| 3 | 3 | ||
| 4 | .DEFAULT_GOAL := help | 4 | .DEFAULT_GOAL := help |
| 5 | -.PHONY: help dev build test test-race lint lint-policy lint-markdown lint-secret-logs lint-spdx lint-unused verify-api-docs fmt tidy clean ci assets install-tools version deploy deploy-check restore-drill bench-staging docs docs-serve docs-verify gen-third-party-notices audit-a11y audit-a11y-pa11y audit-a11y-axe load-test | 5 | +.PHONY: help dev build test test-race lint lint-policy lint-markdown lint-secret-logs lint-spdx lint-unused lint-migrations verify-api-docs fmt tidy clean ci assets install-tools version deploy deploy-check restore-drill bench-staging docs docs-serve docs-verify gen-third-party-notices audit-a11y audit-a11y-pa11y audit-a11y-axe load-test |
| 6 | 6 | ||
| 7 | # Build metadata embedded into the binary via -ldflags. | 7 | # Build metadata embedded into the binary via -ldflags. |
| 8 | VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) | 8 | VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) |
@@ -70,7 +70,7 @@ assets: ## Copy Primer CSS into internal/web/static/ for embedding. | |||
| 70 | echo "warn: .refs/primer-css/dist not found; run 'git clone https://github.com/primer/css .refs/primer-css' first"; \ | 70 | echo "warn: .refs/primer-css/dist not found; run 'git clone https://github.com/primer/css .refs/primer-css' first"; \ |
| 71 | fi | 71 | fi |
| 72 | 72 | ||
| 73 | -ci: lint lint-policy lint-markdown lint-secret-logs lint-spdx lint-unused verify-api-docs test build ## Full CI pipeline (matches .github/workflows/ci.yml). | 73 | +ci: lint lint-policy lint-markdown lint-secret-logs lint-spdx lint-unused lint-migrations verify-api-docs test build ## Full CI pipeline (matches .github/workflows/ci.yml). |
| 74 | @echo "ci: ok" | 74 | @echo "ci: ok" |
| 75 | 75 | ||
| 76 | lint-policy: ## Enforce policy-package boundary (no inline auth checks in handlers/git/cmd). | 76 | lint-policy: ## Enforce policy-package boundary (no inline auth checks in handlers/git/cmd). |
@@ -88,6 +88,9 @@ lint-spdx: ## Verify every Go + shell source carries the SPDX license header. | |||
| 88 | lint-unused: ## Fail when source carries dead-code 'silence unused import' shims (var _ = symbol). | 88 | lint-unused: ## Fail when source carries dead-code 'silence unused import' shims (var _ = symbol). |
| 89 | @scripts/lint-unused.sh | 89 | @scripts/lint-unused.sh |
| 90 | 90 | ||
| 91 | +lint-migrations: ## Fail when goose migration numeric versions collide. | ||
| 92 | + @scripts/lint-migration-versions.sh | ||
| 93 | + | ||
| 91 | verify-api-docs: ## Fail when an /api/v1 route in code is missing from docs/public/api/. | 94 | verify-api-docs: ## Fail when an /api/v1 route in code is missing from docs/public/api/. |
| 92 | @scripts/verify-api-docs.sh | 95 | @scripts/verify-api-docs.sh |
| 93 | 96 | ||
docs/internal/actions-schema.mdmodified@@ -12,7 +12,9 @@ without churning under them. | |||
| 12 | 12 | ||
| 13 | ## SQL schema | 13 | ## SQL schema |
| 14 | 14 | ||
| 15 | -Migrations 0042–0052, in dependency order: | 15 | +Actions migrations currently span 0042–0051 and 0053. Migration 0052 belongs to |
| 16 | +the repo source-remotes feature and was already deployed before the runner JWT | ||
| 17 | +replay table landed. | ||
| 16 | 18 | ||
| 17 | | # | Table | Purpose | | 19 | | # | Table | Purpose | |
| 18 | | ----- | --------------------------- | ------------------------------------------------------------- | | 20 | | ----- | --------------------------- | ------------------------------------------------------------- | |
@@ -26,7 +28,7 @@ Migrations 0042–0052, in dependency order: | |||
| 26 | | 0049 | `actions_variables` | Non-secret per-repo/org config (Forgejo parity) | | 28 | | 0049 | `actions_variables` | Non-secret per-repo/org config (Forgejo parity) | |
| 27 | | 0050 | `workflow_steps.step_with` | Parsed `with:` inputs for magic `uses:` aliases | | 29 | | 0050 | `workflow_steps.step_with` | Parsed `with:` inputs for magic `uses:` aliases | |
| 28 | | 0051 | `workflow_runs.trigger_event_id` | Trigger idempotency for retries/admin replays | | 30 | | 0051 | `workflow_runs.trigger_event_id` | Trigger idempotency for retries/admin replays | |
| 29 | -| 0052 | `runner_jwt_used` | Single-use replay gate for runner job JWTs | | 31 | +| 0053 | `runner_jwt_used` | Single-use replay gate for runner job JWTs | |
| 30 | 32 | ||
| 31 | A few load-bearing choices, called out so they're easy to spot in a | 33 | A few load-bearing choices, called out so they're easy to spot in a |
| 32 | later schema diff: | 34 | later schema diff: |
docs/internal/db.mdmodified@@ -75,7 +75,9 @@ conventions. Every domain sprint (S05 onwards) follows these. | |||
| 75 | convenience but are immutable post-deploy. Corrections come as new | 75 | convenience but are immutable post-deploy. Corrections come as new |
| 76 | migrations. | 76 | migrations. |
| 77 | - One change per migration. Filename: `NNNN_short_purpose.sql` where `NNNN` | 77 | - One change per migration. Filename: `NNNN_short_purpose.sql` where `NNNN` |
| 78 | - is monotonically increasing. | 78 | + is monotonically increasing and globally unique. `scripts/lint-migration-versions.sh` |
| 79 | + enforces this in CI because goose panics on duplicate numeric versions before | ||
| 80 | + it can run any migration. | ||
| 79 | - Goose markers: | 81 | - Goose markers: |
| 80 | 82 | ||
| 81 | ``` | 83 | ``` |
docs/internal/runbooks/actions-runner.mdmodified@@ -5,7 +5,7 @@ operator validation before the real `shithubd-runner` binary lands. | |||
| 5 | 5 | ||
| 6 | Prereqs: | 6 | Prereqs: |
| 7 | 7 | ||
| 8 | -- Database migrations are current through `0052_runner_jwt_used.sql`. | 8 | +- Database migrations are current through `0053_runner_jwt_used.sql`. |
| 9 | - `SHITHUB_TOTP_KEY` or `auth.totp_key_b64` is set on the web process. | 9 | - `SHITHUB_TOTP_KEY` or `auth.totp_key_b64` is set on the web process. |
| 10 | - Object storage is configured if testing artifact upload. | 10 | - Object storage is configured if testing artifact upload. |
| 11 | - A repo has a workflow under `.shithub/workflows/*.yml` with | 11 | - A repo has a workflow under `.shithub/workflows/*.yml` with |
internal/migrationsfs/migrations/0052_runner_jwt_used.sql → internal/migrationsfs/migrations/0053_runner_jwt_used.sqlrenamed (100% similarity)internal/migrationsfs/migrations/0052_push_events_web_protocol.sql → internal/migrationsfs/migrations/0054_push_events_web_protocol.sqlrenamed (100% similarity)internal/migrationsfs/migrationsfs_test.goadded@@ -0,0 +1,54 @@ | |||
| 1 | +// SPDX-License-Identifier: AGPL-3.0-or-later | ||
| 2 | + | ||
| 3 | +package migrationsfs | ||
| 4 | + | ||
| 5 | +import ( | ||
| 6 | + "fmt" | ||
| 7 | + "io/fs" | ||
| 8 | + "sort" | ||
| 9 | + "strings" | ||
| 10 | + "testing" | ||
| 11 | +) | ||
| 12 | + | ||
| 13 | +func TestMigrationVersionsAreUnique(t *testing.T) { | ||
| 14 | + t.Parallel() | ||
| 15 | + | ||
| 16 | + entries, err := fs.ReadDir(FS(), ".") | ||
| 17 | + if err != nil { | ||
| 18 | + t.Fatalf("ReadDir migrations: %v", err) | ||
| 19 | + } | ||
| 20 | + | ||
| 21 | + byVersion := make(map[string][]string) | ||
| 22 | + for _, entry := range entries { | ||
| 23 | + if entry.IsDir() || !isGooseMigrationName(entry.Name()) { | ||
| 24 | + continue | ||
| 25 | + } | ||
| 26 | + version := entry.Name()[:4] | ||
| 27 | + byVersion[version] = append(byVersion[version], entry.Name()) | ||
| 28 | + } | ||
| 29 | + | ||
| 30 | + duplicates := make([]string, 0) | ||
| 31 | + for version, names := range byVersion { | ||
| 32 | + if len(names) <= 1 { | ||
| 33 | + continue | ||
| 34 | + } | ||
| 35 | + sort.Strings(names) | ||
| 36 | + duplicates = append(duplicates, fmt.Sprintf("%s: %s", version, strings.Join(names, ", "))) | ||
| 37 | + } | ||
| 38 | + sort.Strings(duplicates) | ||
| 39 | + if len(duplicates) > 0 { | ||
| 40 | + t.Fatalf("duplicate goose migration versions:\n%s", strings.Join(duplicates, "\n")) | ||
| 41 | + } | ||
| 42 | +} | ||
| 43 | + | ||
| 44 | +func isGooseMigrationName(name string) bool { | ||
| 45 | + if len(name) < len("0000_.sql") || !strings.HasSuffix(name, ".sql") || name[4] != '_' { | ||
| 46 | + return false | ||
| 47 | + } | ||
| 48 | + for _, r := range name[:4] { | ||
| 49 | + if r < '0' || r > '9' { | ||
| 50 | + return false | ||
| 51 | + } | ||
| 52 | + } | ||
| 53 | + return true | ||
| 54 | +} | ||
scripts/lint-migration-versions.shadded@@ -0,0 +1,29 @@ | |||
| 1 | +#!/usr/bin/env bash | ||
| 2 | +# SPDX-License-Identifier: AGPL-3.0-or-later | ||
| 3 | +# | ||
| 4 | +# Goose keys migrations by numeric version, not the rest of the filename. | ||
| 5 | +# Duplicate NNNN prefixes panic at runtime before any migration runs, so | ||
| 6 | +# catch them in CI before a deploy has to discover the conflict. | ||
| 7 | + | ||
| 8 | +set -euo pipefail | ||
| 9 | + | ||
| 10 | +cd "$(git rev-parse --show-toplevel)" | ||
| 11 | + | ||
| 12 | +migrations=$(find internal/migrationsfs/migrations -maxdepth 1 -type f -name '[0-9][0-9][0-9][0-9]_*.sql' | sort) | ||
| 13 | +versions=$(printf '%s\n' "$migrations" | sed -E 's#^.*/([0-9][0-9][0-9][0-9])_.*$#\1#') | ||
| 14 | +duplicates=$(printf '%s\n' "$versions" | sort | uniq -d) | ||
| 15 | + | ||
| 16 | +if [ -n "$duplicates" ]; then | ||
| 17 | + echo "lint-migration-versions: duplicate migration versions found:" >&2 | ||
| 18 | + echo >&2 | ||
| 19 | + while IFS= read -r version; do | ||
| 20 | + [ -z "$version" ] && continue | ||
| 21 | + echo " version $version:" >&2 | ||
| 22 | + printf '%s\n' "$migrations" | grep "/${version}_" | sed 's/^/ /' >&2 | ||
| 23 | + done <<< "$duplicates" | ||
| 24 | + echo >&2 | ||
| 25 | + echo "Goose requires each numeric NNNN prefix to be globally unique." >&2 | ||
| 26 | + exit 1 | ||
| 27 | +fi | ||
| 28 | + | ||
| 29 | +echo "lint-migration-versions: ok" | ||