feat(cli): report state backend versions on rollback and info - #6088
Conversation
`sqlmesh rollback` printed nothing at all, so there was no way to tell what it had actually done. That is worst precisely when it matters: part way through a bad upgrade, with no command anywhere that shows the version state of the state backend. Rollback now reports the schema, SQLGlot and SQLMesh versions it moved from and to, once the rollback has succeeded, and `sqlmesh info -v` reports the backend's current versions. Both read through one helper so the two commands cannot drift apart, and `info` degrades to an error message rather than failing when the versions can't be read. The versions are read with validate=False, since the point is to show what is there, including the mismatched state that prompts a rollback in the first place. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
@cmgoffena13 — closes #6045. Two calls I had to make without an answer, both noted on the issue and both easy to flip:
Say if you'd rather have either the other way round. |
|
@tripleaceme -- this was missed in the issue, but could you apply this logic to I'm good with only showing on success. Any error will produce proper output. Lets keep the versions in One edge case, Easy fix in Before: if not migrate_rows and major_minor(SQLMESH_VERSION) == versions.minor_sqlmesh_version:
return
if migrate_rows:
self._migrate_rows(promoted_snapshots_only)
self.version_state.update_versions()After: if not migrate_rows and major_minor(SQLMESH_VERSION) == versions.minor_sqlmesh_version:
if (
versions.sqlmesh_version != SQLMESH_VERSION
or versions.sqlglot_version != SQLGLOT_VERSION
):
self.version_state.update_versions()
return
if migrate_rows:
self._migrate_rows(promoted_snapshots_only)
self.version_state.update_versions() |
Review feedback on SQLMesh#6088. `migrate` was as silent as `rollback` was, so it now reports the same before and after versions once it succeeds. It also fixes a case where that output would have been wrong. The early return in StateSyncMigrator.migrate compares only MAJOR.MINOR, so a patch-level upgrade left the recorded versions behind what was actually running and `migrate` reported no change. The recorded versions are now brought up to date in that branch when they differ from the running ones. Both are covered: one test asserts the before and after pair is printed, and one pins the patch bump specifically. The second sets both recorded minor versions equal to the installed ones on purpose — with a differing minor, `_apply_migrations` reports rows to migrate and the early return under test is never reached. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Thanks @cmgoffena13 — both done in 8005529, and noted on the two defaults staying as they are.
Your patch-bump fix is in, applied as you wrote it, with a comment saying why the branch exists. One thing worth flagging about the test for it, because my first attempt was wrong and passed for the wrong reason. I initially seeded Tests added to
The three |
|
@tripleaceme -- okay, one last small fix: for the migrator we should actually call the version update with the same schema_version. Like this: |
Review feedback on SQLMesh#6088. The patch-bump branch called update_versions with no arguments, and schema_version defaults to the current SCHEMA_VERSION, so a run with nothing to migrate still moved it. The recorded value is now carried over, since a migration that is genuinely still needed must not be masked by a version bump that skipped it. The guarantee is pinned in tests/core/state_sync, not through the CLI: a state whose schema version differs makes _apply_migrations report rows to migrate, so the branch is unreachable end to end. The test forces _apply_migrations to report nothing, which is the only way to reach it, and that is also why the change is defensive rather than a fix for something observable today. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Done in 8882ae8 — Worth recording why, since it took me a moment to see it: Where the test had to go. I first tried to pin this through the CLI by seeding an older schema version, and it failed — a differing schema version makes So the guarantee is pinned in def test_migrate_patch_bump_preserves_schema_version(state_sync, mocker):
...
mocker.patch(
"sqlmesh.core.state_sync.db.migrator.StateMigrator._apply_migrations",
return_value=False,
)
state_sync.migrate()
assert versions.schema_version == stale_schema_versionVerified it fails without the change ( The three |
SQLMesh#6088 makes rollback report the versions it moved between, so the note would go stale as soon as that merges. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
@tripleaceme -- looks like |
info -v and migrate now print the state backend versions, so the magics tests see four more lines each. The migrate test builds the expected values from the running versions rather than hard-coding them. The sushi state is an in-memory DuckDB database, so the state sync migrate opens starts empty and the versions move from the defaults. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Fixed in cbfa9c0. Both tests now expect the four extra lines.
One thing worth knowing about what these tests show. Both print the versions moving from the defaults ( |
Description
Closes #6045.
sqlmesh rollbackprinted nothing at all, so there was no way to tell what it had done. That is worst exactly when it matters most: part way through a bad upgrade, with no command anywhere that shows the version state of the state backend.Rollback now reports the schema, SQLGlot and SQLMesh versions it moved from and to:
And
sqlmesh info -vreports the backend's current versions, so there is a way to inspect them without performing a rollback.Both go through one helper, so the two commands cannot drift apart, and
infologs an error rather than failing outright if the versions can't be read. The versions are read withvalidate=False, since the whole point is to show what is actually there — including the mismatched state that prompts a rollback in the first place.Two defaults I picked, both flagged on the issue and neither answered, so say the word if you'd rather have them the other way:
infoshows the versions behind-v, as suggested in the issue, rather than at default verbosity.Test Plan
Three tests in
tests/cli/test_cli.py:test_info_state_versions— plaininfodoes not print them;info -vdoes, and the values matchSCHEMA_VERSION/ the installed SQLGlot / the installed SQLMesh.test_rollback_state_versions— backs up state, fakes a migration to a newer version, then asserts the rollback prints84 -> 83style before/after pairs for all three.test_rollback_without_backup_does_not_print_state_versions— a rollback that fails with "There are no prior migrations to roll back to." prints no versions. This one guards the ordering: it fails if the print is ever moved ahead of the rollback call.I checked the first two fail without the implementation, so they aren't passing for free.
The
test_dlt_*andtest_pyspark_python_modelfailures are pre-existing onmainin my environment (ModuleNotFoundError).Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO