feat: support for structured scaffold in create command #970
Conversation
for more information, see https://pre-commit.ci
|
@pre-commit-ci[bot] is attempting to deploy a commit to the sparckles Team on Vercel. A member of the Team first needs to authorize it. |
CodSpeed Performance ReportMerging #970 will not alter performanceComparing Summary
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
ashupednekar
changed the title
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Sanskar Jethi <[email protected]>
| ├── migrations | ||
| │ ├── README | ||
| │ ├── env.py | ||
| │ ├── script.py.mako |
There was a problem hiding this comment.
I need to read more about this
| with patch("robyn.cli.os.makedirs") as mock_makedirs: | ||
| with patch("robyn.cli.shutil.copytree") as mock_copytree, patch("robyn.os.remove") as _mock_remove: |
There was a problem hiding this comment.
didn't follow...
added additional key in mock return value scaffold_type
and added two tests for the two possible values
| @router.get("/livez/") | ||
| def livez() -> str: | ||
| return "live" | ||
|
|
||
|
|
||
| @router.get("/healthz/") | ||
| def healthz() -> str: |
There was a problem hiding this comment.
let's use more formal names here like
subrouter_healthcheck and subrouter_livecheck
| class SampleHandlers: | ||
| """ | ||
| note: the handles being grouped in a class like this is complete optional, and doesn't have any impact on routing | ||
| """ | ||
|
|
||
| @router.post("/one") | ||
| @staticmethod | ||
| def one(): ... | ||
|
|
||
| @router.get("/two") | ||
| @staticmethod | ||
| def two(): ... |
There was a problem hiding this comment.
not a big fan of this syntax honestly.
There was a problem hiding this comment.
should I remove the sample handlers class? it doesn't really serve any purpose as such.
| from robyn.helpers import discover_routes | ||
|
|
||
| app: Robyn = discover_routes("api.handlers") |
| ? Directory Path: myproject | ||
| ? Need Docker? (Y/N) Y | ||
| ? Please choose if you'd like the scaffold to be a simple starter kit or an opinionated structure | ||
| Simple |
There was a problem hiding this comment.
@ashupednekar , I don't see an update here
| COPY . . | ||
| RUN pip install --no-cache-dir --upgrade -r requirements.txt --target=/workspace/deps | ||
|
|
||
| FROM gcr.io/distroless/python3-debian11 |
There was a problem hiding this comment.
could you explain a bit more here?
There was a problem hiding this comment.
This is a two stage commit...
- takes python base image as builder
- copies source and does pip install in this stage
- second stage is gcr distroless debian 11 (https:/GoogleContainerTools/distroless)
- distroless is a minimal image meant for production usage which is regularly maintained and patched for security vulnerabilities, and comes with an added benefit of small size as it doesn't include stuff from base linux image that's not needed at runtime
- we copy the site-packages dir, which is /workspace/deps in our case so that our dependencies are available
- finally, we set pythonpath and add server.py to the image's CMD
| from robyn.helpers import discover_routes | ||
| from robyn import Robyn | ||
|
|
||
| from utils.db import get_pool | ||
| from conf import settings | ||
|
|
||
| app: Robyn = discover_routes("api.handlers") |
There was a problem hiding this comment.
we don't have a discover_routes function
There was a problem hiding this comment.
It's added in helpers as part of this PR...
def discover_routes(handler_path: str = "api.handlers") -> Robyn:
mux: Robyn = Robyn(__file__)
package = importlib.import_module(handler_path)
for _, module_name, _ in pkgutil.iter_modules(package.__path__, package.__name__ + "."):
module = importlib.import_module(module_name)
logger.info(f"member: {module}")
mux.include_router(module.router)
return mux
| from .user import Base as user | ||
|
|
||
| metadata = [user.metadata] |
There was a problem hiding this comment.
yes, https://alembic.sqlalchemy.org/en/latest/autogenerate.html
alembic expects the model's sqlalchemy metadata to be passed in env.py
having it in models/init.py for better structure
which is used in alembic/env.py like so
from adaptors import models
target_metadata = models.metadata
| class Config: | ||
| orm_mode = True |
There was a problem hiding this comment.
the config? nope.... it's pydantic model config, it's supposed to be defined inline in the class definition
https://docs.pydantic.dev/1.10/usage/model_config/
There was a problem hiding this comment.
could you mention it in the code please?
| level = WARN | ||
| handlers = console | ||
| qualname = | ||
|
|
||
| [logger_sqlalchemy] | ||
| level = WARN | ||
| handlers = | ||
| qualname = sqlalchemy.engine | ||
|
|
||
| [logger_alembic] | ||
| level = INFO | ||
| handlers = | ||
| qualname = alembic |
There was a problem hiding this comment.
what do the empty assignments mean here?
There was a problem hiding this comment.
These are alembic defaults, generated with alemic init migrations
The reason I chose to pre-run this and not leave it to users is so that we can make `migrations/env.py' changes to make is usable right away
Also, to standardize migrations directory structure
There was a problem hiding this comment.
@ashupednekar , since alembic is not a tool where I have direct experience. Could you add a readme?
There was a problem hiding this comment.
Should I add it in the scaffold, or should I add more docs ?
I've added what's needed to get started under example_app/index.mdx
| # are written from script.py.mako | ||
| # output_encoding = utf-8 | ||
|
|
||
| sqlalchemy.url = driver://user:pass@localhost/dbname |
There was a problem hiding this comment.
let us leave a comment, which says that you need to update it accordingly.
There was a problem hiding this comment.
Done
Also, there was a bug in the online migrations function in alembic/env.py in structured sqlalchemy scaffold. Fixed, and added docs in example_app/index.mdx for database migration steps as well
| COPY . . | ||
| RUN pip install --no-cache-dir --upgrade -r requirements.txt --target=/workspace/deps | ||
|
|
||
| FROM gcr.io/distroless/python3-debian11 |
There was a problem hiding this comment.
the --target flag tells pip to place the site packages at the specified path, we then copy this in the second stage and set PYTHONPATH accordingly
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Description
This PR fixes #969
Summary
PR: Add Structured Scaffold Option to Robyn's Create Command
Description:
This PR introduces a new feature to Robyn's
createcommand, allowing users to choose between two scaffold options:Changes:
createcommand to prompt users with a new option:Would you like the scaffold to be a simple starter kit or an opinionated structure?api/handlers(for route handlers)middlewares(for middleware definitions)adaptors(withmodels,selectors,mutators)utils(helper functions likedb.py)devops(including Dockerfile, docker-compose)conf.py,config.env).Example Structured Scaffold:
├── no-db │ ├── api │ │ ├── handlers │ │ │ ├── __init__.py │ │ │ ├── probes.py │ │ │ └── sample.py │ │ └── middlewares │ │ └── __init__.py │ ├── conf.py │ ├── config.env │ ├── devops │ │ ├── Dockerfile │ │ ├── Dockerfile.src │ │ └── docker-compose.yaml │ ├── requirements.txt │ ├── server.py │ └── utils │ └── __init__.py └── sqlalchemy ├── adaptors │ ├── __init__.py │ ├── models.py │ ├── mutators │ │ └── __init__.py │ ├── schema.py │ └── selectors │ └── __init__.py ├── api │ ├── handlers │ │ ├── __init__.py │ │ ├── probes.py │ │ └── sample.py │ └── middlewares │ └── __init__.py ├── conf.py ├── config.env ├── devops │ ├── Dockerfile │ ├── Dockerfile.src │ └── docker-compose.yaml ├── requirements.txt ├── server.py └── utils ├── __init__.py └── db.pyPR Checklist
Please ensure that:
Pre-Commit Instructions: