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

Run database migrations after pulling changes #377

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

HebaruSan
Copy link
Contributor

Motivation

After #369 was merged, the alpha server was left in a semi-functional state because its alembic migration wasn't executed. This command fixed it (I used the migration id in place of head, but I want to document the best version of it):

cd /SpaceDock
bin/alembic upgrade head

This happens automatically when you run ./start-server.sh, via ./spacedock database migrate:

spacedock database migrate &&

... which does this:

SpaceDock/spacedock

Lines 12 to 15 in ebb54cb

def _get_alembic_config():
if os.path.isfile('alembic.ini'):
from alembic.config import Config
return Config('alembic.ini')

SpaceDock/spacedock

Lines 75 to 79 in ebb54cb

cfg = _get_alembic_config()
if cfg is not None:
from alembic import command
site_logger.info('Migrating database...')
command.upgrade(cfg, 'head')

Changes

Now after the web hook pulls changes from GitHub, and before it restarts the sever, it also tries to run the alembic migrations.

@HebaruSan HebaruSan added Type: Improvement Area: Backend Related to the Python code that runs inside gunicorn Type: Question Priority: Low Status: In Progress Status: Need Feedback Area: Migration Related to Alembic database migrations labels Jul 18, 2021
@HebaruSan
Copy link
Contributor Author

Will that work?

@DasSkelett
Copy link
Member

After all these years! Nice to see this finally happening. Not to say I would've done this years ago, but... I'd have done this years ago.

Will that work?

Hard to say, testing this code section is a bit hard. I did run a manual

alembic.command.downgrade(alembic.config.Config('alembic.ini'), '426e0b848d77')
alembic.command.upgrade(alembic.config.Config('alembic.ini'), 'head')

which worked just fine (except one unrelated issue, see below). I don't see why it wouldn't in the celery task.

Also while running this on a new table, I realized that the new primary key of mod_followers from #369 isn't called pk_mod_followers but mod_followers_pkey if generated from scratch instead of the migration. This made the downgrade fail.
Appending name='pk_mod_followers' to the PrimaryKeyConstraint declaration should fix that.

Not a huge bug, it's unlikely that someone downgrades newly created databases except for testing, but it's nice to have things consistent. Do you want to add this to this PR, since we're already in the area of db migrations?

@HebaruSan
Copy link
Contributor Author

Good catch, fixed.

@DasSkelett
Copy link
Member

Found some more bugs while testing #357 and related to #369, but put them in my own separate PR, since they touch more code now that doesn't have anything to do with this PR.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll only find out whether it works once we try it out on alpha, so let's give it a go.

@HebaruSan HebaruSan merged commit 3197cf7 into KSP-SpaceDock:alpha Jul 29, 2021
@HebaruSan HebaruSan deleted the feature/auto-db-migrate branch July 29, 2021 17:03
@DasSkelett
Copy link
Member

As far as I can tell it worked perfectly after merging #374:

$ sudo -u www-data bin/alembic current
833ba1ee8c72 (head)

This was referenced Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Backend Related to the Python code that runs inside gunicorn Area: Migration Related to Alembic database migrations Priority: Low Status: In Progress Status: Need Feedback Type: Improvement Type: Question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants