Skip to content

adds CockroachDB as an option to the Piccolo playground - #1229

Merged
dantownsend merged 5 commits into
piccolo-orm:masterfrom
sinisaos:cockroach_to_playground
Jul 29, 2025
Merged

adds CockroachDB as an option to the Piccolo playground#1229
dantownsend merged 5 commits into
piccolo-orm:masterfrom
sinisaos:cockroach_to_playground

Conversation

@sinisaos

Copy link
Copy Markdown
Member

Related to #1228. Also add orjson to the Lilya asgi template as it seems that orjson is now a hard dependency for Lilya (without that change our integration test would fail for Lilya with ModuleNotFoundError: No module named 'orjson').

@sinisaos
sinisaos requested a review from dantownsend July 23, 2025 06:48

@dantownsend dantownsend left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good 👍

Comment thread piccolo/apps/asgi/commands/new.py Outdated
"litestar": ["litestar"],
"esmerald": ["esmerald"],
"lilya": ["lilya"],
"lilya": ["lilya", "orjson"],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting that lilya would do this without having it as one of their own dependencies, but I'm OK with adding this if it works. 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree with you that Lilya should have its own dependencies, because even lilya[all] (install everything) doesn't work. We should leave this as is to satisfy our integration tests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread piccolo/apps/playground/commands/run.py Outdated
"database": database,
"user": "root",
"password": "",
"port": 26257,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Port is kind of problematic, because we do allow the user to pass it in as an arg. I think we need to make port a nullable argument, which defaults to None. And if not passed in, then it's 5432 for Postgres and 26257 for Cockroach.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought the default Cockroach port was enough (in playground we run CockroachDB in single node with the insecure flag and that is highlighted in the docs) because playground is only for testing or learning Piccolo. Do you think it could be port optional with something like this or?

def run(
    ...
    port: Optional[int] = None,
    ...
):
    if engine.upper() == "POSTGRES":
        db: Engine = PostgresEngine(
            {
                ...
                "port": port if port is not None else 5432,
            }
        )
    elif engine.upper() == "COCKROACH":
        db = CockroachEngine(
            {
                ...
                "port": port if port is not None else 26257,
            }
        )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I think we should give the user the option to specify the port. So like you've done, except we can simplify it:

def run(
    ...
    port: Optional[int] = None,
    ...
):
    if engine.upper() == "POSTGRES":
        db: Engine = PostgresEngine(
            {
                ...
                "port": port or 5432,
            }
        )
    elif engine.upper() == "COCKROACH":
        db = CockroachEngine(
            {
                ...
                "port": port or 26257,
            }
        )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, I'll change that. Thanks.

Comment thread piccolo/apps/playground/commands/run.py Outdated
"host": host,
"database": database,
"user": "root",
"password": "",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have a password param too, so we might need to do the same as with port - make it optional, and if not specified then default to an empty string?

Or in the docs tell them to create the database with the same password as Postgres (piccolo).

Comment thread piccolo/apps/playground/commands/run.py Outdated
Comment on lines 301 to 302
:param user:
Postgres user

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need to update these params - e.g. Postgres/CockroachDB user.

Or Database user (ignored for SQLite)?

@dantownsend

Copy link
Copy Markdown
Member

Tested it, and it works great 👍

Will merge in once CI finishes.

@dantownsend
dantownsend merged commit 93cc9ce into piccolo-orm:master Jul 29, 2025
46 checks passed
@sinisaos
sinisaos deleted the cockroach_to_playground branch July 29, 2025 07:24
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