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

Fix alembic autogeneration and rename mismatching constraints #39032

Merged
merged 14 commits into from
May 4, 2024

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Apr 15, 2024

The alembic autogeneration is not working as expected and the tests were not detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the database for tests, the changes in the ORM will appear the same with what is in the migration file. To be sure that both match, we have to compare the database generated using the migration file to the database that could be created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was done in the migration file instead of the ORM as I take the ORM as the source of truth. New airflow users create their DB from the ORM with the correct naming because we have a naming convention. Old airflow users would have to upgrade to use these names from ORM instead of the reverse.

I also removed the sqlite_sequence table which is specific to sqlite and not needed for anything. An alternative would be to add sqlite_autoincrement to table args in the ORM and migration but this table is not that useful.

@ephraimbuddy ephraimbuddy force-pushed the fix-autogenerate branch 2 times, most recently from e540be2 to 0d7c1f6 Compare April 16, 2024 08:22
@ephraimbuddy ephraimbuddy added the full tests needed We need to run full set of tests for this PR to merge label Apr 16, 2024
@uranusjr
Copy link
Member

The test_database_schema_and_sqlalchemy_model_are_in_sync test does not seem to agree with this…

@ephraimbuddy
Copy link
Contributor Author

The test_database_schema_and_sqlalchemy_model_are_in_sync test does not seem to agree with this…

Should be fixed now

airflow/models/dag.py Outdated Show resolved Hide resolved
@ephraimbuddy
Copy link
Contributor Author

Previously, I added the use_migration_files as just a parameter to the DB upgrade/reset functions. Now, I have extended it to be a CLI option so one can run airflow db reset --use-migration-files if they want to. I think that's how it should be

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.
@ephraimbuddy ephraimbuddy merged commit 00f0969 into apache:main May 4, 2024
68 checks passed
@ephraimbuddy ephraimbuddy deleted the fix-autogenerate branch May 4, 2024 07:06
RodrigoGanancia pushed a commit to RodrigoGanancia/airflow that referenced this pull request May 10, 2024
…#39032)

* Fix alembic autogeneration and rename mismatching constraints

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.

* fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* Fix mysql, sqlite and issue with cascading deletes

* fixup! Fix mysql, sqlite and issue with cascading deletes

* Fix migration for mysql

* Fix clear_number ORM server_default

* Fix type change for mysql

* Fix constraints for sqlite and move migration to 2.9.2

* Fix sqlite constraints update and ignore session_session_id_uq index

* Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* fixup! Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* Apply suggestions from code review

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@utkarsharma2 utkarsharma2 added the type:improvement Changelog: Improvements label Jun 3, 2024
@ephraimbuddy ephraimbuddy added type:bug-fix Changelog: Bug Fixes and removed type:improvement Changelog: Improvements labels Jun 4, 2024
utkarsharma2 pushed a commit that referenced this pull request Jun 4, 2024
* Fix alembic autogeneration and rename mismatching constraints

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.

* fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* Fix mysql, sqlite and issue with cascading deletes

* fixup! Fix mysql, sqlite and issue with cascading deletes

* Fix migration for mysql

* Fix clear_number ORM server_default

* Fix type change for mysql

* Fix constraints for sqlite and move migration to 2.9.2

* Fix sqlite constraints update and ignore session_session_id_uq index

* Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* fixup! Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* Apply suggestions from code review

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit 00f0969)
ephraimbuddy added a commit that referenced this pull request Jun 5, 2024
* Fix alembic autogeneration and rename mismatching constraints

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.

* fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* Fix mysql, sqlite and issue with cascading deletes

* fixup! Fix mysql, sqlite and issue with cascading deletes

* Fix migration for mysql

* Fix clear_number ORM server_default

* Fix type change for mysql

* Fix constraints for sqlite and move migration to 2.9.2

* Fix sqlite constraints update and ignore session_session_id_uq index

* Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* fixup! Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* Apply suggestions from code review

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit 00f0969)
ephraimbuddy added a commit that referenced this pull request Jun 5, 2024
* Fix alembic autogeneration and rename mismatching constraints

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.

* fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* Fix mysql, sqlite and issue with cascading deletes

* fixup! Fix mysql, sqlite and issue with cascading deletes

* Fix migration for mysql

* Fix clear_number ORM server_default

* Fix type change for mysql

* Fix constraints for sqlite and move migration to 2.9.2

* Fix sqlite constraints update and ignore session_session_id_uq index

* Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* fixup! Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* Apply suggestions from code review

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit 00f0969)
ephraimbuddy added a commit that referenced this pull request Jun 5, 2024
* Fix alembic autogeneration and rename mismatching constraints

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.

* fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* Fix mysql, sqlite and issue with cascading deletes

* fixup! Fix mysql, sqlite and issue with cascading deletes

* Fix migration for mysql

* Fix clear_number ORM server_default

* Fix type change for mysql

* Fix constraints for sqlite and move migration to 2.9.2

* Fix sqlite constraints update and ignore session_session_id_uq index

* Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* fixup! Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* Apply suggestions from code review

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit 00f0969)
utkarsharma2 pushed a commit that referenced this pull request Jun 5, 2024
* Fix alembic autogeneration and rename mismatching constraints

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.

* fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* Fix mysql, sqlite and issue with cascading deletes

* fixup! Fix mysql, sqlite and issue with cascading deletes

* Fix migration for mysql

* Fix clear_number ORM server_default

* Fix type change for mysql

* Fix constraints for sqlite and move migration to 2.9.2

* Fix sqlite constraints update and ignore session_session_id_uq index

* Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* fixup! Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* Apply suggestions from code review

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit 00f0969)
ephraimbuddy added a commit that referenced this pull request Jun 5, 2024
* Fix alembic autogeneration and rename mismatching constraints

The alembic autogeneration is not working as expected and the tests
were detecting it because we use DBs created from the ORM to run tests.
When a change is made in the ORM and the ORM is used to initialize the
database for tests, the changes in the ORM will appear the same with what
is in the migration file. To be sure that both match, we have to compare
the database generated using the migration file to the database that could be
created from the ORM.
To fix this, I added 'use_migration_file' arg to resetdb function and updated
the db reset in conftest to use migration file during test db reset.

As part of this fix, I also updated mismatching constraint names. The update was
done in the migration file instead of the ORM as I take the ORM as the source
of truth. New airflow users create their DB from the ORM with the correct naming
because we have a naming convention. Old airflow users would have to upgrade
to use these names from ORM instead of the reverse.

I also removed the `sqlite_sequence` table which is specific to sqlite and not
needed for anything. An alternative would be to add `sqlite_autoincrement` to
table args in the ORM and migration but this table is not that useful.

* fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* fixup! fixup! fixup! Fix alembic autogeneration and rename mismatching constraints

* Fix mysql, sqlite and issue with cascading deletes

* fixup! Fix mysql, sqlite and issue with cascading deletes

* Fix migration for mysql

* Fix clear_number ORM server_default

* Fix type change for mysql

* Fix constraints for sqlite and move migration to 2.9.2

* Fix sqlite constraints update and ignore session_session_id_uq index

* Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* fixup! Fix processor_subdir in the migration file for mysql and make use-migration-files an option in db commands

* Apply suggestions from code review

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit 00f0969)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:db-migrations PRs with DB migration full tests needed We need to run full set of tests for this PR to merge kind:documentation type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants