Skip to content

Pending migrations check on startup. Includes index change migration#4746

Open
matthewmcgarvey wants to merge 3 commits intoiv-org:masterfrom
matthewmcgarvey:pending-migration-check
Open

Pending migrations check on startup. Includes index change migration#4746
matthewmcgarvey wants to merge 3 commits intoiv-org:masterfrom
matthewmcgarvey:pending-migration-check

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
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