-
-
Notifications
You must be signed in to change notification settings - Fork 882
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 custom migration runner, forbid some diesel migration
commands, fix old migrations
#4673
base: main
Are you sure you want to change the base?
Conversation
diesel migration
no longer allowed)
diesel migration
no longer allowed)diesel migration
commands no longer allowed)
diesel migration
commands no longer allowed)diesel migration
commands, fix old migrations
Sry about being slow to review this, I'd like it to come after the |
…n Cargo.lock` then letting it auto update again
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.
The editing (even older broken migrations) seems a bit scary (but you did explain why you need to do this.). And I don't understand the reason for all of the custom code for diff checking.
But overall I think this is a good idea.
when: *slow_check_paths | ||
|
||
check_diesel_migration: |
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.
We can't check / test the migrations anymore in CI?
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 was moved to the unit tests in schema_setup.rs
@@ -220,14 +221,36 @@ BEGIN | |||
post_aggregates_view | |||
WHERE | |||
id = NEW.post_id; | |||
-- Force the hot rank as zero on week-older posts | |||
-- Update the comment hot_ranks as of last week | |||
UPDATE |
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.
These are really old migrations that are updating tables and views that aren't used anymore, whats going on?
I've wanted to split this PR so the changed diff behavior and the migration changes it requires are separate, but I've been putting it off. I will do that soon. For the migration changes, I now think the diff checker should only check migrations after the migration that removes all views. That should allow removing most of the migration changes. I will work on this for a while after I make the PR with the simpler changes. |
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.
Seems fine, but I'll need to test this out with a production DB locally, to make sure there are no issues.
Reverting and testing migrations will be done with
lemmy_server migration
commands, anddiesel migration
commands will do nothing and fail when trying to run or revert migrations.Benefits:
All migrations, not just the newest one, will be tested with schema diff(moved to Improve migration diff check and fix old migrations #5204)#4333 (comment)
The new tests (even just reverting all migrations) revealed some mistakes in old migrations, so I fixed those, and I also discovered that'now'::timestamp
in old views used the timestamp of the view's creation instead of the current timestamp.