-
Notifications
You must be signed in to change notification settings - Fork 0
CLI: enroll_db #23
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
CLI: enroll_db #23
Conversation
Hey @algal and @jph00, thanks so much for the reviews and feedback of #22! Looking at this PR now I see in hindsight the scope got way too large. This is a near total rewrite of that PR based on your awesome commentary. Thanks to the schema code I did have to go over the requested line count. The issue there is making sure the initial migration is guaranteed to work on an existing SQLite db. I'm keen to learn of any methods I can use to cut down the line count further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new CLI command “enroll_db” that enrolls an existing SQLite database into fastmigrate’s versioning system by creating an initial migration file and initializing the _meta table. Key changes include:
- Adding the enroll_db CLI command and its integration in fastmigrate/cli.py.
- Creating tests in tests/test_cli.py to verify various enroll_db scenarios.
- Updating configuration in pyproject.toml and enhancing helper functions in fastmigrate/core.py.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/test_cli.py | Added tests to cover successful enrollments, error cases, and config file usage. |
pyproject.toml | Registered the new CLI command for enroll_db. |
fastmigrate/core.py | Enhanced _ensure_meta_table output and added get_db_schema functionality. |
fastmigrate/cli.py | Introduced the enroll_db command that creates an initial migration and enrolls the DB. |
README.md | Updated documentation to include the enroll_db command usage. |
cursor = conn.execute("SELECT name FROM users WHERE name='test_user'") | ||
assert cursor.fetchone() is not None | ||
|
||
# Version should be 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Clarify in the test and/or inline comment that the enroll_db command applies an initial migration to increment the database version from 0 to 1. This will help reviewers understand that _ensure_meta_table initially sets version to 0 and the migration is responsible for updating it.
# Version should be 0 | |
# Verify that the version is 1 after the initial migration. | |
# `_ensure_meta_table` sets the version to 0, and `enroll_db` increments it to 1. |
Copilot uses AI. Check for mistakes.
db_path, migrations_path = _get_config(config_path, db, migrations) | ||
if not migrations_path.exists(): migrations_path.mkdir(parents=True) | ||
initial_migration = migrations_path / "0001-initial.sql" | ||
schema = core.get_db_schema(db_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding a comment that explains how the current database schema is used to generate the initial migration file, ensuring users understand that any subsequent changes to the DB schema are not reflected in this file.
schema = core.get_db_schema(db_path) | |
schema = core.get_db_schema(db_path) | |
# The initial migration file is generated based on the current database schema. | |
# Note: Any subsequent changes to the database schema will not be reflected in this file. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better, thanks! :)
For the schema, how about this?:
from apsw.shell import Shell
from io import StringIO
out = StringIO()
shell = Shell(stdout=out, db=db.conn)
shell.process_command('.schema')
print(out.getvalue())
Implementing this approach knocked that function from 20 LOC to 8! 🔥 |
@algal AFAIK this is ready to merge, but please double-check and merge it if you're happy with it. |
IIUC we can close #14 when this is merged. |
@jph00 I'll handle this today with other fastmigrate release work. |
This PR adds a CLI command for enrolling a DB, adding an initial migration in the process.
enroll_db
, which adds the_meta
versioning table, as well as including an initial migration called0001-initial.sql