-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Manual Migration Feature #3220
Conversation
Passing run #1990 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3220 +/- ##
==========================================
- Coverage 86.95% 86.89% -0.07%
==========================================
Files 307 308 +1
Lines 18460 18525 +65
Branches 2407 2420 +13
==========================================
+ Hits 16052 16097 +45
- Misses 1970 1988 +18
- Partials 438 440 +2
☔ View full report in Codecov by Sentry. |
…ub.com/ethyca/fides into ThomasLaPiana-manual-migration-feature
@pattisdr This is ready for your review/opinions! Sorry it took so long for me to circle back and put the finishing touches on it Honestly I'm still iffy on the implementation/UX, given the caveat that it needs to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a nice addition, I think it's fine if it needs to be true on startup. The main thing I thought would be helpful here is for when you're trying to figure out a custom migration and to keep it from auto-migrating on you all the time.
@pattisdr Thank you for the great catches here! Addressed the comments, ready for another review 🙂 |
@pattisdr tossing back to you! Lmk if there are any big issues here, otherwise I'd prefer to get this merged and iterate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @ThomasLaPiana
Closes #3093
Code Changes
database
setting forautomigrate
init
tomigrate
when referring to the database migration steps, since that feels more accurate and less surprising to medeprecation
label onfides db init
and point people tofides db migrate
migrate
CLI commandautomigrate
config value when doing migrationsmain.py
and move some of the verbose setup toapp_setup.py
Steps to Confirm
nox -s teardown -- volumes
to clear the databaseautomigrate = false
to thefides.toml
in the[database]
sectionnox -s dev
and see that it doesn't run the database migrations and subsequently fails startupfides.toml
that disables auto-migrationsnox -s dev
again and see that everything works (it will also log anINFO
statement that migrations are being run)automigrate = false
nox -s dev
again and see that it doesn't run migrations, as evidenced by a logINFO
stating that migrations are being skipped and lack of migration loggingPre-Merge Checklist
Relevant Follow-Up Issues CreatedCHANGELOG.md
Description Of Changes
Add the ability for users to turn off auto-migrations. Also rename
init
tomigrate
everywhere since it makes more sense (in my opinion) and is used for more than just initializing.Breaking Changes
The
CLI
won't see any breaks here, as theinit
command has been deprecated instead of removed.However, the
db/<action>
endpoint has been broken, as it no longer acceptsinit
. This is reflected in the autogenerated documentation though since it uses a properEnum
for the actions.