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

aleph CLI command to downgrade the postgres DB #3858

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

stchris
Copy link
Contributor

@stchris stchris commented Aug 28, 2024

No description provided.

@@ -24,7 +24,7 @@

def channel_tag(obj, clazz=None):
clazz = clazz or type(obj)
if clazz == str:
if clazz is str:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not related, but my local ruff installation complained about it 🤷

Copy link
Contributor

@tillprochaska tillprochaska left a comment

Choose a reason for hiding this comment

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

Thank you, works as expected :) Added one suggestion about a dry-run option below.

aleph/manage.py Outdated
Comment on lines 507 to 517
@cli.command()
@click.option(
"--revision",
"-r",
default="-1",
show_default=True,
help="target revision number. Can also be 'head' or '-1'",
)
def downgrade_database(revision):
"""Downgrade the database."""
downgrade_db(revision)
Copy link
Contributor

@tillprochaska tillprochaska Aug 29, 2024

Choose a reason for hiding this comment

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

Running database downgrades can be quite scary as they can delete data. The core alembic CLI has a dry-run option and I just checked and Flask-Migrate exposes that, too.

Suggested change
@cli.command()
@click.option(
"--revision",
"-r",
default="-1",
show_default=True,
help="target revision number. Can also be 'head' or '-1'",
)
def downgrade_database(revision):
"""Downgrade the database."""
downgrade_db(revision)
@cli.command()
@click.option(
"--revision",
"-r",
default="-1",
show_default=True,
help="target revision number. Can also be 'head' or '-1'",
)
@click.option(
"--sql",
default=False,
is_flag=True,
help="...",
)
def downgrade_database(revision, sql):
"""Downgrade the database."""
downgrade_db(revision, sql)

Something like this should print the SQL statements the migration would execute without actually executing it, as well as the names of the migrations that will be rolled back.

This would allow us to double check that the downgrade is doing what we expect before actually executing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I tested this locally, I accidentally rolled back a different migration than I expected… 😄

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 made these changes:

  • the default is a dry run, which prints a warning that this doesn't actually execute, the migration and the SQL statements and quits.
  • you now have to pass --execute to actually run the migrations.

Remove debug print

Remove debug print statements

Add an explicit execute flag, default to dry run
@stchris stchris merged commit ab3981e into release/4.0.0 Sep 18, 2024
2 checks passed
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