HYPERFLEET-975 - fix: resolve diff base via merge-base#123
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe script hack/verify-migrations.sh now selects the base commit for migration diffs conditionally: if running in a merge-commit context (detects HEAD^2) it uses HEAD^1 as BASE; else if origin/main exists it uses git merge-base HEAD origin/main; otherwise it prints a “no PR detected … skipping” message and exits 0. Comments describing migration immutability and additive-schema conventions were moved earlier in the script. The migration violation detection (git diff over migration .go files matching MRCD, excluding migration_structs.go) and failure behavior are unchanged. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 1
🤖 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 29-35: The current merge-commit check (git rev-parse --verify
--quiet HEAD^2) can be true in non-PR CI runs; update the guard to require a
real PR context first (e.g. GITHUB_EVENT_NAME == "pull_request" or
GITHUB_HEAD_REF non-empty for GitHub Actions, or CI-specific vars like CHANGE_ID
/ CI_MERGE_REQUEST_IID / PULL_REQUEST for other CI) before using git rev-parse
HEAD^2 to set BASE=$(git rev-parse HEAD^1); if those PR-specific environment
variables are not present, fall back to the existing origin/main logic or skip
with the same "no PR detected" message to avoid computing diffs for
rehearsals/extra_refs jobs.
🪄 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: 92dcd9c5-daa8-45be-a6c0-a1616ac9c880
📒 Files selected for processing (1)
hack/verify-migrations.sh
kuudori
force-pushed
the
HYPERFLEET-975
branch
2 times, most recently
from
1fb3632 to
aeae6b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/db/migrations/202604230001_add_nodepool_owner_deleted_index.go`:
- Line 16: The migration file name and the migration ID inside it disagree: the
file is named 202604230001_add_nodepool_owner_deleted_index.go while the
migration struct uses ID "202604230003", which breaks the filename/ID convention
and the chronological ordering in the MigrationList; fix by making them
consistent — either rename the file to
202604230003_add_nodepool_owner_deleted_index.go so the prefix matches the ID,
or change the migration ID string to "202604230001" and ensure the entry for
this migration in MigrationList is positioned after addReconciledIndex() (the
function with ID "202604211859") so all IDs are strictly ascending for
gormigrate.
🪄 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: e68aed34-c680-46ce-ac2b-e1e854b34851
📒 Files selected for processing (2)
hack/verify-migrations.shpkg/db/migrations/202604230001_add_nodepool_owner_deleted_index.go
✅ Files skipped from review due to trivial changes (1)
- hack/verify-migrations.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/db/migrations/202604230003_add_nodepool_owner_deleted_index.go`:
- Around line 14-20: The migration addNodePoolOwnerDeletedIndextest (ID
"202604230002") duplicates the index created by the earlier migration that
defines idx_node_pools_owner_deleted; remove this duplicate migration and its
entry from the MigrationList (or if you intend a different change, give this
migration a new unique ID and change the DDL to a genuinely different schema
alteration). Locate the function addNodePoolOwnerDeletedIndextest and the
MigrationList where it's registered, then either delete the function and its
registration or modify the ID and SQL to perform a distinct change instead of
recreating idx_node_pools_owner_deleted.
🪄 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: 83daeb67-e907-49b1-8415-0e9c5a1e3998
📒 Files selected for processing (3)
hack/verify-migrations.shpkg/db/migrations/202604230003_add_nodepool_owner_deleted_index.gopkg/db/migrations/migration_structs.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/verify-migrations.sh
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 1d45541
into
openshift-hyperfleet:main
2 participants
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit