HYPERFLEET-975 - ci: add migration immutability check#114
HYPERFLEET-975 - ci: add migration immutability check#114kuudori wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds a migration immutability check: a new script Sequence Diagram(s)sequenceDiagram
participant CI
participant Makefile
participant Script as hack/verify-migrations.sh
participant Git
participant TestRunner
CI->>Makefile: invoke "verify-all"
Makefile->>Script: run hack/verify-migrations.sh
Script->>Git: determine base (PULL_BASE_SHA or git merge-base)
Script->>Git: diff pkg/db/migrations/*.go (exclude migration_structs.go)
Git-->>Script: list of changed migration files (or none)
alt violations found
Script-->>Makefile: exit 1 + failure message
Makefile-->>CI: abort verify-all
else no violations
Script-->>Makefile: success
Makefile->>TestRunner: continue with remaining verify-all steps (lint, test)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/verify-migrations.sh`:
- Around line 12-15: The current immutability check assigns VIOLATIONS by
diffing all "${MIGRATION_DIR}/*.go", which incorrectly freezes
migration_structs.go (which must be edited to append new entries to
MigrationList); update the VIOLATIONS computation (the assignment referencing
VIOLATIONS and MIGRATION_DIR) to only include timestamped migration files (e.g.,
match the filename pattern used for individual migrations) or explicitly exclude
migration_structs.go so that additive changes can update MigrationList without
failing the check.
In `@Makefile`:
- Line 202: The ci-test-unit Makefile target is missing the verify-migrations
dependency, so running CI with ci-test-unit bypasses hack/verify-migrations.sh;
update the ci-test-unit target to include verify-migrations (same way the test
target does) so that ci-test-unit depends on verify-migrations and thus runs
hack/verify-migrations.sh before executing tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f9818caa-70b5-41d1-9e3f-0f997e09a0ce
📒 Files selected for processing (2)
Makefilehack/verify-migrations.sh
|
/retest |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/verify-migrations.sh (1)
10-10: Add explicit error handling for the base resolution command.Line 10 can fail abruptly under
set -ewhen thegit merge-basecommand fails—whether due to a missingorigin/mainor no common ancestry. Without an explicit guard, the script terminates without guidance. Add error handling with a clear message:Suggested change
-BASE="${PULL_BASE_SHA:-$(git merge-base HEAD origin/main)}" +if [[ -n "${PULL_BASE_SHA:-}" ]]; then + BASE="${PULL_BASE_SHA}" +else + if ! BASE="$(git merge-base HEAD origin/main 2>/dev/null)"; then + echo "FAIL: unable to resolve base commit. Ensure origin/main exists and HEAD has common history, or set PULL_BASE_SHA." >&2 + exit 1 + fi +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/verify-migrations.sh` at line 10, The BASE assignment using PULL_BASE_SHA and git merge-base can fail under set -e; update the BASE resolution in hack/verify-migrations.sh to run the git merge-base command with explicit error handling: run the command, capture its output into BASE (or a temp var), check if it succeeded and that BASE is non-empty, and if it failed print a clear error mentioning git merge-base and origin/main (or missing PULL_BASE_SHA) and exit non-zero; ensure references to BASE and the git merge-base invocation (the current BASE="${PULL_BASE_SHA:-$(git merge-base HEAD origin/main)}" logic) are updated so failures produce a helpful message rather than an abrupt script termination.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/verify-migrations.sh`:
- Line 10: The BASE assignment using PULL_BASE_SHA and git merge-base can fail
under set -e; update the BASE resolution in hack/verify-migrations.sh to run the
git merge-base command with explicit error handling: run the command, capture
its output into BASE (or a temp var), check if it succeeded and that BASE is
non-empty, and if it failed print a clear error mentioning git merge-base and
origin/main (or missing PULL_BASE_SHA) and exit non-zero; ensure references to
BASE and the git merge-base invocation (the current BASE="${PULL_BASE_SHA:-$(git
merge-base HEAD origin/main)}" logic) are updated so failures produce a helpful
message rather than an abrupt script termination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5fc01df9-90f8-46ea-85e6-86345919ecb3
📒 Files selected for processing (2)
Makefilehack/verify-migrations.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
1 participant
Add verify-migrations.sh that detects modifications to existing migration files by diffing against the PR base commit.
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit
Tests
Chores