ENG-2881 Raise error on startup when migrations fail#7562
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe pull request adds error handling to abort server startup when database migrations fail. An exception during migration is caught, logged in detail, and re-raised as a FidesError to prevent the server from starting in an inconsistent state. A changelog entry documents this behavior change. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR addresses a correctness issue where database migration failures during server startup were silently logged, allowing the server to continue running in an inconsistent state. The fix converts the silent logging approach into a hard error that aborts startup. Key changes:
What's good:
Confidence Score: 4/5
Last reviewed commit: 0b8bd68 |
adamsachs
left a comment
There was a problem hiding this comment.
looks good, thanks for taking this on! 🙏
the main risk i see here is just around expectation setting/understanding implications for deployment. i know we've communicated with our platform team, so that's a good start. i'm thinking that in addition to that, it's probably worth:
- calling that out again to the platform team to make sure this is fully consistent + good with our deployment approach on our cloud
- make sure we're over-communicating about it for customer-hosted deployments (e.g. it needs to be high up in release notes, probably worth some explicit heads-up on slack, etc.)
i don't anticipate any red flags, but this is a fairly consequential change in app behavior (a positive one!) that we should be sure to give heads-up on.
Ticket ENG-2881
Description Of Changes
Currently when migrations run on startup and fail for whatever reason, the server still starts up normally. This means application code is deployed and “healthy”, but it assumes the migrations ran successfully (e.g assumes a new table was created when it wasn’t) , leading to errors and a generally inconsistent state.
We should simply raise the error when migrations fail.
Steps to Confirm
raise Exception('error')code, or otherwise break your migrations (e.g removing the latest migration file)output from my test:
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit