feat(ci): add migration-police for migration filenames - #4433
Merged
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇✅ ✅ Action performedFull review finished. |
chet
changed the title
Contributor
Summary by CodeRabbit
WalkthroughThe change defines a timestamped naming format for new Core database migrations. A Bash checker validates newly added migration filenames. Pull-request CI runs the checker and requires its result in ChangesMigration filename validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant MigrationChecker
participant CoreCIPass
PullRequest->>GitHubActions: Open or update pull request
GitHubActions->>MigrationChecker: Run with origin/main as base
MigrationChecker->>MigrationChecker: Validate newly added migration filenames
MigrationChecker-->>GitHubActions: Return validation status
GitHubActions->>CoreCIPass: Report migration-police status
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chet
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-migration-filenames.sh`:
- Around line 53-61: Update the migration filename validation in
check-migration-filenames.sh to validate the complete basename against the
required YYYYMMDDhhmmss_description.sql format before checking for the trailing
0000 placeholder. Preserve the existing placeholder error behavior for otherwise
valid filenames, and reject malformed new migration filenames under the
documented contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a98aff7-dc45-4e7b-b44b-b1002261d87f
📒 Files selected for processing (4)
.github/workflows/ci.yamlAGENTS.mdSTYLE_GUIDE.mdscripts/check-migration-filenames.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Line 1370: Update the workflow’s push branch pattern to match pull-request
branches under pull-request/ using GitHub Actions glob syntax, such as
pull-request/**, while preserving the existing github.ref condition for the
push-based flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aab48b08-8832-46b3-83c7-67893426a848
📒 Files selected for processing (4)
.github/workflows/ci.yamlAGENTS.mdSTYLE_GUIDE.mdscripts/check-migration-filenames.sh
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇✅ ✅ Action performedFull review finished. |
lachen-nv
approved these changes
Jul 31, 2026
nick-vetro
approved these changes
Jul 31, 2026
poroh
approved these changes
Jul 31, 2026
chet
enabled auto-merge (squash)
July 31, 2026 06:08
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 NVIDIA#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 NVIDIA#4431 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇✅ ✅ Action performedFull review finished. |
nick-vetro
approved these changes
Jul 31, 2026
polarweasel
pushed a commit
to polarweasel/infra-controller
that referenced
this pull request
Jul 31, 2026
This enforces that migration files have their timestamp fully populated, and not end in `0000`. Core migrations have more recently been using `...120000` placeholder timestamps, which means two independently authored PRs can both look fine until their versions collide on `main` -- exactly what NVIDIA#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. ## Related issues This supports NVIDIA#4431 ## Type of Change - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) Focused checks covered a valid full timestamp, a rejected `0000` placeholder, an accepted zero-seconds timestamp with a populated minute, historical migrations against `origin/main`, an invalid base revision, shell syntax, workflow YAML parsing, and `git diff --check`. The required local gates also passed: - `cargo make format-nightly` - `cargo make clippy` - `cargo make carbide-lints` ## Additional Notes `cargo test -p carbide-api-db migration_versions_are_unique --lib` currently fails on the pre-existing duplicate `20260722120001` migration version. NVIDIA#4432 repairs that collision and reports the migration test passing. Signed-off-by: Chet Nichols III <chetn@nvidia.com> Signed-off-by: Alex Ball <aball@nvidia.com>
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
4 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.
What happened
The failures on #4422 and #4424 looked flaky because they surfaced as a very large test cascade, but the underlying failure was deterministic: SQLx migration versions are keyed by the numeric timestamp prefix, and two different migration files had the same version.
On #4422, these files both used
20260722120000:20260722120000_bmc_suppressions.sql20260722120000_preserve_machine_ipv6_loopback.sqlThat made template-database migration fail on the
_sqlx_migrations_pkeyconstraint. Every database-backed test that tried to create a template then failed from the same setup error, producing 1,114 failures rather than 1,114 independent test bugs.Two stale fixes later composed on #4424 and moved both files to
20260722120001, recreating the same collision under a different version. #4432 repairedmainby restoring the already-published loopback migration to20260722120000and leaving the newer BMC migration at20260722120001; the SQL contents themselves did not change.What this changes
This adds a separate
migration-policejob that looks only at migration files newly added by a pull request and rejects timestamp prefixes whose minute and second fields are both00. New Core migrations must use a fully populatedYYYYMMDDhhmmss_description.sqltimestamp.The existing duplicate-version test remains the definitive backstop once migrations are in the same tree. This earlier CI rule reduces the chance that independent branches choose the same convenient
...0000placeholder before they can be compared together. Existing migrations remain accepted.The checker compares the PR head with the merge base (
base...HEAD), so a stale PR does not incorrectly claim that a migration renamed only onmainwas added by the PR.STYLE_GUIDE.mdandAGENTS.mddocument the same naming rule.Related issues
Fixes #4431
Related failure investigations: #4422, #4424
Type of Change
Breaking Changes
Testing
Focused checks covered:
0000minute/second placeholdermaingit diff --checkcargo test -p carbide-api-db migration_versions_are_unique --libThe required local gates passed:
cargo make format-nightlycargo make clippycargo make carbide-lintsLocal CodeRabbit produced no findings. Claude review identified the stale-PR two-dot diff problem; the checker was changed to merge-base diff semantics and the stale #4422 branch was used as a focused regression case. Cloud CodeRabbit subsequently completed a full review of the amended commit with no actionable findings.
Additional Notes
The immediate migration collision is already repaired on current
mainby #4432. This PR prevents the naming pattern that allowed two independent branches to select the same placeholder timestamp; it does not rename published migrations.Closes #4431