Skip to content

Fix: add validators to handle None values in Versions class#1654

Merged
georgesittas merged 4 commits intomainfrom
jo/schema_version_fix
Nov 2, 2023
Merged

Fix: add validators to handle None values in Versions class#1654
georgesittas merged 4 commits intomainfrom
jo/schema_version_fix

Conversation

@georgesittas
Copy link
Contributor

...sigh

Copy link
Contributor

@tobymao tobymao left a comment

Choose a reason for hiding this comment

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

can you make a test that actually migrated from n-1 to n?

@georgesittas
Copy link
Contributor Author

I can try, maybe I need to mock out the schema version so that it doesn't pick v0032 or something

@georgesittas georgesittas requested a review from tobymao November 2, 2023 03:28
@georgesittas
Copy link
Contributor Author

Added a unit test for this.

def test_migrate_only_to_add_sqlmesh_version_column(duck_conn, monkeypatch) -> None:
from sqlmesh import __version__ as SQLMESH_VERSION

n_migrations_before_sqlmesh_version = 31
Copy link
Contributor

Choose a reason for hiding this comment

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

make this n -1 so we always test the latest migration right

Copy link
Contributor Author

@georgesittas georgesittas Nov 2, 2023

Choose a reason for hiding this comment

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

It's not that simple because this test has logic specific to the 31 -> 32 migration, i.e. patching _update_versions and assuming that prior to that migration we don't have sqlmesh_version in state.

If we generalized this the above sections would not be valid e.g. for n-1 to n and the test would need additional guards etc

@georgesittas georgesittas merged commit 8435c44 into main Nov 2, 2023
@georgesittas georgesittas deleted the jo/schema_version_fix branch November 2, 2023 16:01
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.

2 participants