Built-in Database Migration Tool For Robyn #1205
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
✨ No issues found! Your code is sparkling clean! ✨ Need help? Join our Discord for support! |
CodSpeed Performance ReportMerging #1205 will not alter performanceComparing Summary
|
|
acknowledging the PR 😄 Will need to learn a few things before review |
| parser.add_argument( | ||
| "db", | ||
| nargs="?", | ||
| default=None, | ||
| help="Database migration commands. Use 'robyn db' to see more information.", | ||
| ) |
There was a problem hiding this comment.
@Charlie-BU , this supposed to invoke alembic cli?
There was a problem hiding this comment.
yeah, but the alembic cli is being encapsulated in migrate.py. The arguments passed by robyn and that passed by alembic are not exactly the same, so i need to preprocess the robyn cli argument to make it available to alembic. The details are all in migrate.py.
Here, it just adds a positional argument for robyn. Then user would be able to revoke the encapsulated alembic cli using robyn db ..., which being handled here:
| @@ -3,6 +3,7 @@ | |||
| import subprocess | |||
| import sys | |||
| import webbrowser | |||
| import argparse | |||
There was a problem hiding this comment.
I think we need this to define a sub-argument for robyn, which is robyn db .... We might need argparse to process the argument passed after robyn db.
robyn/cli.py
Outdated
| if db_migration == "Y": | ||
| print("Installing the latest version of alembic...") | ||
| try: | ||
| subprocess.run([sys.executable, "-m", "pip", "install", "alembic", "-q"], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) |
There was a problem hiding this comment.
there should be a better way here, no?
There was a problem hiding this comment.
I was considering to add alembic as a dependency when people execute pip install robyn. But in view that not everyone needs this database migration tool, I decided to make it an option for people when they do robyn create. If they said yes then alembic is a must in this case.
I modified this self-installing process like this:
Pls check if there's something that could be optimized. Thx! :D
| def catch_errors(f): | ||
| """Decorator to catch and handle errors.""" | ||
|
|
||
| @wraps(f) | ||
| def wrapped(*args, **kwargs): | ||
| try: | ||
| return f(*args, **kwargs) | ||
| except Exception as e: | ||
| print(f"ERROR: {str(e)}") | ||
| sys.exit(1) | ||
|
|
||
| return wrapped |
There was a problem hiding this comment.
I don't like this pattern tbh. We should have specific exception handling. not a generic catch all at the library level.
There was a problem hiding this comment.
Let me think how to refactor this...😊
| if template is None: | ||
| return Path(__file__).parent / "templates" / "robyn" | ||
| return Path(template) |
There was a problem hiding this comment.
This path validation might be incorrect.
There was a problem hiding this comment.
Could you possibly explain more? I think it works.
robyn/migrate.py
Outdated
| import_statement = f"sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))\nfrom {module_path} import {class_name}\ntarget_metadata = {class_name}.metadata" | ||
|
|
||
| # Replace the import statement | ||
| content = content.replace( | ||
| "# add your model's MetaData object here\n# for 'autogenerate' support\n# from myapp import mymodel\n# target_metadata = mymodel.Base.metadata", | ||
| f"# add your model's MetaData object here\n# for 'autogenerate' support\n{import_statement}", | ||
| ) | ||
|
|
||
| # Replace the target_metadata setting | ||
| content = content.replace( | ||
| "target_metadata = config.attributes.get('sqlalchemy.metadata', None)", | ||
| "# target_metadata = config.attributes.get('sqlalchemy.metadata', None)\n# Already set by the import above", |
There was a problem hiding this comment.
This env.py file and alembic.ini file are automatically created by alembic when handling alembic init alembic, which is robyn db init now. And we have to modify the configuration here to adapt it to the real case.
There was a problem hiding this comment.
might be better to create a db module. This file will be incredible hard to test otherwise.
robyn/templates/robyn/env.py
Outdated
There was a problem hiding this comment.
would need some more explanation about this file 😅
|
Hey @Charlie-BU 👋 Thanks for the PR. This is a good start :D But I have some inline comments. |
for more information, see https://pre-commit.ci
Description
This PR adds a built-in database migration tool to Robyn, powered by Alembic. It introduces a new
robyn dbCLI command group that provides a full suite of migration capabilities, includinginit,migrate,upgrade,downgrade,stamp, and more.Summary
This PR does the following:
robyn db) for SQLAlchemy-based projectsalembic.iniandenv.pybased on user input or intelligent detection (e.g., database URL, model metadata path)init,revision,migrate,upgrade,downgrade,merge,edit,stamp,show,history, etc.--template robyn)Migratethat can be optionally used inside Robyn apps for future integrationThis aims to make database versioning and schema evolution first-class citizens in the Robyn developer workflow, reducing friction for backend developers.
PR Checklist
Please ensure that:
Pre-Commit Instructions: