add check command for upgrade diffs #1101

nxlouie · 2022-10-17T23:52:29Z

Fixes #724

Description

Add check command. If the revision command with autogenerate has pending upgrade operations to run, then raise an error. Otherwise, continue.

Testing

Wrote Tests:

tox tests/test_command.py::CheckTest::test_check_no_changes
tox tests/test_command.py::CheckTest::test_check_changes_detected

I also tested this function in my company Replica's code base:

from alembic.config import Config
from alembic.command import check

def test_migration_autogen_parity():
    alembic_cfg = Config("alembic.ini")
    check(alembic_cfg)

When there is parity, the test passes.
When I spike a model with an extra change and then run the test, the error is thrown:

>           raise util.RevisionOpsNotEmptyError(f"Revision has upgrade ops to run: {diffs}.")
E           alembic.util.exc.RevisionOpsNotEmptyError: Revision has upgrade ops to run: [('add_column', 'schema_name', 'table_name', Column('new_column_name', TEXT(), table=<table_name>))].

.venv/lib/python3.10/site-packages/alembic/command.py:292: RevisionOpsNotEmptyError
...
Results (16.42s):
       1 failed
         - migrations/test_migration_autogen_parity.py:7 test_migration_autogen_parity

Have a nice day!

zzzeek · 2022-10-18T00:01:48Z

OK two things needed:

a test in test_command.py
shouldn't this be it's own command? alembic check calling it "alembic revision" is a little unclear because with the --check option, a revision would never be generated.

nxlouie · 2022-10-18T04:50:31Z

Hey @zzzeek,

Added a few tests. Apologies for submitting this early - I put it in draft mode and set out to figure out the testing framework, meaning to set as ready when I had the tests. I understand that generates extra noise for the maintainers.
I set out to follow the proposal in Enable comparing migrations to implementation in tests. #724 by @ziima verbatim:

I'm glad to hear that. Here's my proposal: add --check option to revision command. It would print the differences in migrations (using logging, since alembic has no verbosity) and return non-zero exit code, if it found any. It would be only available when --autogenerate is used. Other command options are applied accordingly.

If I understand correctly, it was to mirror the django makemigrations --check behavior.
Do we still want to follow that? If not, I can leave an update on the issue and implement a separate command.

zzzeek · 2022-11-07T19:50:45Z

I think "revision --check" makes no sense for us as it means "revision" is not going to ever generate a revision, "check" is better. @CaselIT opinions?

CaselIT · 2022-11-07T19:59:00Z

I think "revision --check" makes no sense for us as it means "revision" is not going to ever generate a revision, "check" is better. @CaselIT opinions?

I do prefer check since using revision --check without augenerate would be basically a no-op, while we can document that check implies autogenerate mode (or reflection in any case)

nxlouie · 2022-11-07T20:12:51Z

Sounds good, I'll give it a shot.

This reverts commit 2be534c.

This reverts commit 7c4d160.

nxlouie · 2022-11-08T07:01:40Z

@zzzeek @CaselIT - I updated this PR to be a distinct check command. In addition to the tests written here, I can confirm that a variant of this code has successfully been running in my company Replica's code base for the past month, and that when I plugged in this check command, the test caught the non-empty revision ops.

VenturaParade · 2022-12-12T14:13:13Z

Hello everyone, I am also waiting for this to be merged to use this feature, any idea when it could be?

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 090b594 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-12-12T14:54:32Z

New Gerrit review created for change 090b594: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4295

zzzeek · 2022-12-12T14:54:59Z

we have very little time right now to work on alembic and this PR was forgotten. will see if I can keep an eye on it in the coming weeks and maybe get it merged

zzzeek · 2022-12-13T01:56:44Z

the test seems to not be passing.

nxlouie · 2022-12-13T07:30:30Z

Hey @zzzeek - thanks for running the tests. I put in fixes for the test_help_text and pep8 failures.

However, I don't know what to do with the openstack_gerrit errors. Let me know if you think it's unrelated or if there's particular action I should take with them.

CaselIT · 2022-12-13T08:19:04Z

Hey @zzzeek - thanks for running the tests. I put in fixes for the test_help_text and pep8 failures.

However, I don't know what to do with the openstack_gerrit errors. Let me know if you think it's unrelated or if there's particular action I should take with them.

These failures are most likely unrelated

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision fbe8f10 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-12-13T08:19:13Z

Patchset fbe8f10 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4295

zzzeek · 2022-12-13T13:30:20Z

yes dont worry about the openstack stuff, those tests frquently stop working due to downstream changes and i usualyl dont have time to keep fixing them

sqla-tester · 2022-12-13T14:35:06Z

mike bayer (zzzeek) wrote:

looks like you're down to pep8 stuff. install pre-commit https://pre-commit.com/ in the repo and that will run the code formatters.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4295

nxlouie · 2022-12-13T16:30:24Z

@zzzeek Thanks for the pointers. I ran pre-commit run --files tests/test_command.py alembic/command.py alembic/util/__init__.py alembic/util/exc.py and put in the fixes from black

sqla-tester

Hi, this is sqla-tester and I see you've pinged me for review. However, user nxlouie is not authorized to initiate CI jobs. Please wait for a project member to do this!

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 807ed54 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-12-13T17:58:10Z

Patchset 807ed54 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4295

sqla-tester · 2022-12-13T19:49:50Z

mike bayer (zzzeek) wrote:

@CaselIT can you give this a final review? i made some changes in this patchset. will call this 1.9.0

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4295

zzzeek · 2022-12-13T19:51:06Z

I've made a series of final changes that are visible only on the gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4295 . feel free to review!

nxlouie · 2022-12-13T21:48:56Z

Awesome, The documentation looks good from my end!

sqla-tester · 2022-12-15T18:01:48Z

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4295 has been merged. Congratulations! :)

VenturaParade · 2022-12-15T18:38:52Z

Thanks a lot for the quick work :)

add check flag and check upgrade diffs

7c4d160

add tests

2be534c

nxlouie marked this pull request as ready for review October 18, 2022 04:43

nxlouie added 4 commits November 7, 2022 22:04

Revert "add tests"

9997976

This reverts commit 2be534c.

Revert "add check flag and check upgrade diffs"

4b25a58

This reverts commit 7c4d160.

make check command

509fa22

update return type to None

090b594

nxlouie changed the title ~~add check flag and check upgrade diffs~~ add check command for upgrade diffs Nov 9, 2022

zzzeek requested a review from sqla-tester December 12, 2022 14:54

sqla-tester reviewed Dec 12, 2022

View reviewed changes

nxlouie added 2 commits December 12, 2022 23:15

flake8 fix

6f87e1f

test_help_text fix

fbe8f10

CaselIT requested a review from sqla-tester December 13, 2022 08:19

sqla-tester reviewed Dec 13, 2022

View reviewed changes

pre-commit run

807ed54

nxlouie requested a review from sqla-tester December 13, 2022 17:51

sqla-tester reviewed Dec 13, 2022

View reviewed changes

CaselIT requested a review from sqla-tester December 13, 2022 17:58

sqla-tester reviewed Dec 13, 2022

View reviewed changes

sqlalchemy-bot closed this in 4678d7f Dec 15, 2022

nxlouie deleted the add-check-flag-to-revision-command branch December 15, 2022 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add check command for upgrade diffs #1101

add check command for upgrade diffs #1101

nxlouie commented Oct 17, 2022 •

edited

zzzeek commented Oct 18, 2022

nxlouie commented Oct 18, 2022 •

edited

zzzeek commented Nov 7, 2022

CaselIT commented Nov 7, 2022 •

edited

nxlouie commented Nov 7, 2022

nxlouie commented Nov 8, 2022

VenturaParade commented Dec 12, 2022

sqla-tester left a comment

sqla-tester commented Dec 12, 2022

zzzeek commented Dec 12, 2022

zzzeek commented Dec 13, 2022

nxlouie commented Dec 13, 2022 •

edited

CaselIT commented Dec 13, 2022

sqla-tester left a comment

sqla-tester commented Dec 13, 2022

zzzeek commented Dec 13, 2022

sqla-tester commented Dec 13, 2022

nxlouie commented Dec 13, 2022 •

edited

sqla-tester left a comment

sqla-tester left a comment

sqla-tester commented Dec 13, 2022

sqla-tester commented Dec 13, 2022

zzzeek commented Dec 13, 2022

nxlouie commented Dec 13, 2022

sqla-tester commented Dec 15, 2022

VenturaParade commented Dec 15, 2022

add check command for upgrade diffs #1101

add check command for upgrade diffs #1101

Conversation

nxlouie commented Oct 17, 2022 • edited

Description

Testing

Wrote Tests:

I also tested this function in my company Replica's code base:

zzzeek commented Oct 18, 2022

nxlouie commented Oct 18, 2022 • edited

zzzeek commented Nov 7, 2022

CaselIT commented Nov 7, 2022 • edited

nxlouie commented Nov 7, 2022

nxlouie commented Nov 8, 2022

VenturaParade commented Dec 12, 2022

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Dec 12, 2022

zzzeek commented Dec 12, 2022

zzzeek commented Dec 13, 2022

nxlouie commented Dec 13, 2022 • edited

CaselIT commented Dec 13, 2022

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Dec 13, 2022

zzzeek commented Dec 13, 2022

sqla-tester commented Dec 13, 2022

nxlouie commented Dec 13, 2022 • edited

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Dec 13, 2022

sqla-tester commented Dec 13, 2022

zzzeek commented Dec 13, 2022

nxlouie commented Dec 13, 2022

sqla-tester commented Dec 15, 2022

VenturaParade commented Dec 15, 2022

nxlouie commented Oct 17, 2022 •

edited

nxlouie commented Oct 18, 2022 •

edited

CaselIT commented Nov 7, 2022 •

edited

nxlouie commented Dec 13, 2022 •

edited

nxlouie commented Dec 13, 2022 •

edited