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

chore: Improve Alembic Migration Generation #4192

Merged

Conversation

michael-genson
Copy link
Collaborator

What type of PR is this?

(REQUIRED)

  • cleanup

What this PR does / why we need it:

(REQUIRED)

This PR modifies Alembic to make generating migrations easier. It makes the following changes:

  • Adds some type hints to make the linter quit whining (even though we don't actually enforce these in migration scripts)
  • Modifies the migration template to output linter-friendly migrations
  • Adds exclusions to skip known false migrations (e.g. removing unique constraints that we don't want to remove)
  • Defaults to batch mode (so migrations should work as-is for SQLite)

I also updated the database-changes docs to remove the blurb about batch operations, since that's now done automatically.

Which issue(s) this PR fixes:

(REQUIRED)

N/A

Testing

(fill-in or delete this section)

I created a migration using these changes and confirmed:

  1. The migration is empty (no undesired changes)
  2. The migration is fully formatted/linted

@github-actions github-actions bot added the chore label Sep 10, 2024
@@ -202,8 +202,6 @@ def downgrade():
)
op.drop_index(op.f("ix_recipes_rating"), table_name="recipes")
op.alter_column("recipes", "rating", existing_type=sa.Float(), type_=sa.INTEGER(), existing_nullable=True)
op.create_unique_constraint("ingredient_units_name_group_id_key", "ingredient_units", ["name", "group_id"])
op.create_unique_constraint("ingredient_foods_name_group_id_key", "ingredient_foods", ["name", "group_id"])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Other than type hints, this is the only change made to actual migration code. This is in the downgrade method though, which we don't use. These lines shouldn't be there.

@boc-the-git
Copy link
Collaborator

Is there any regression testing that can be/was done on this?
I haven't fully reviewed it as yet (from a skim, the changes look mostly trivial), but I note your testing notes mostly relate to forward looking issues.

@boc-the-git boc-the-git self-requested a review September 16, 2024 10:43
@boc-the-git boc-the-git self-assigned this Sep 16, 2024
Copy link
Collaborator

@boc-the-git boc-the-git left a comment

Choose a reason for hiding this comment

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

LGTM - will be great to have Alembic be a smoother process! 🚀

Couple comments.

alembic/script.py.mako Outdated Show resolved Hide resolved
@michael-genson
Copy link
Collaborator Author

Regarding regression testing - the closest thing we have is some migration tests. We have test data from various migrations which are all run against the existing migration scripts, and we check some known previous data issues.

So since test pass we know older versions can still migrate to the latest version, and that the data isn't messed up (in specific instances)

@michael-genson michael-genson enabled auto-merge (squash) September 16, 2024 13:44
@michael-genson michael-genson merged commit 8778559 into mealie-recipes:mealie-next Sep 16, 2024
13 checks passed
boc-the-git pushed a commit to boc-the-git/mealie that referenced this pull request Sep 28, 2024
boc-the-git pushed a commit to boc-the-git/mealie that referenced this pull request Sep 28, 2024
Choromanski pushed a commit to Choromanski/mealie that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants