-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Pending migrations check on startup. Includes index change migration #4746
base: master
Are you sure you want to change the base?
Pending migrations check on startup. Includes index change migration #4746
Conversation
@@ -135,6 +135,9 @@ end | |||
OUTPUT = CONFIG.output.upcase == "STDOUT" ? STDOUT : File.open(CONFIG.output, mode: "a") | |||
LOGGER = Invidious::LogHandler.new(OUTPUT, CONFIG.log_level) | |||
|
|||
# Check pending migrations | |||
Invidious::Database::Migrator.new(PG_DB).check_pending_migrations | |||
|
|||
# Check table integrity | |||
Invidious::Database.check_integrity(CONFIG) |
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.
I noticed this method while working... and this is a bit of a monster hidden behind a simple name. It performs database updates besides doing checks. This might have to be taken out completely once the migration files are for sure the way forward since it reads the sql files.
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.
Yeah, this is a remnant I haven't dare to touch. I think that it should be safe to remove all of that code.
One thing I just remembered is that, while currently no migrations are marked as required, it would probably be wise to mark most of them so that people get on the migration train. |
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.
Looks good to me!
We could only mark the latest as required, that'd be enough. |
Thank you! I'll look into doing some guides for applying migrations manually. |
Need to update this PR to also drop the index that this new one replaces if it exists. |
When the app starts, it will check for any pending migrations and issue a warning (but allow the app to continue). Adds new functionality to migrations to mark them as required which, during the pending migration check, will cause the app to fail to start.
To test this, I took the migration from #2469 and included it here. It will not break anything if the migration is not run and seems useful for performance issues.
I know some are pushing towards automatically running migrations, but I am not. Feel free to push back