Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

matthewmcgarvey
Copy link
Contributor

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

@matthewmcgarvey matthewmcgarvey requested a review from a team as a code owner June 12, 2024 01:00
@matthewmcgarvey matthewmcgarvey requested review from unixfox and removed request for a team June 12, 2024 01:00
@@ -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)
Copy link
Contributor Author

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.

Copy link
Member

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.

@matthewmcgarvey
Copy link
Contributor Author

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.

Copy link
Member

@SamantazFox SamantazFox left a 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!

@SamantazFox
Copy link
Member

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.

We could only mark the latest as required, that'd be enough.
Though we need to agree on a process for docker users.

@unixfox
Copy link
Member

unixfox commented Jun 18, 2024

Thank you! I'll look into doing some guides for applying migrations manually.

@matthewmcgarvey
Copy link
Contributor Author

Need to update this PR to also drop the index that this new one replaces if it exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants