Skip to content
This repository was archived by the owner on Dec 11, 2021. It is now read-only.

Move to asyncpg instead of psycopg#29

Merged
ChrisLovering merged 8 commits into
mainfrom
async-sqlalch
Nov 15, 2021
Merged

Move to asyncpg instead of psycopg#29
ChrisLovering merged 8 commits into
mainfrom
async-sqlalch

Conversation

@ChrisLovering
Copy link
Copy Markdown
Member

This PR moves our migrations to asyncpg, adds a docker volume for migrations and adds a helper task for generating those migrations.

Simply by running poetry run task revision "Migration message here." while the docker containers are running, a migration will be generated.

Something to keep in mind is that, now that we are using asyncpg, no database validation can be run within Pydantic models. This now needs to be deferred to the routes that need that validation instead.

Comment thread pyproject.toml Outdated
@D0rs4n
Copy link
Copy Markdown
Contributor

D0rs4n commented Nov 11, 2021

Looks fine, the bogus DATABASE_URL in the CI has to be changed. It was added because of the Pydantic configuration, I'll resolve all of those problems in my next PR, but it has to be updated in order for the tests to pass.
Thank you!

Comment thread pyproject.toml Outdated

[tool.taskipy.tasks]
lint = "pre-commit run --all-files"
revision = "docker-compose exec web alembic revision --autogenerate -m" No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This doesn't work if we are running the database locally (without docker).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It works, if you add the database url of your local database to an '.env' file , hence the alembic environment uses the pydantic config.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How would it work? It runs docker-compose exec, I am not runnning with docker...

Copy link
Copy Markdown
Contributor

@D0rs4n D0rs4n Nov 12, 2021

Choose a reason for hiding this comment

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

Oh, yes sorry.
However, I think if you run your database locally ( which is way harder than just issuing a simple command, but let's go with the example) you can just simply do poetry run alembic revision [...] (and of course setting the env vars) The PR and this task was meant to solve the problem of creating migrations while using docker, that's why the migrations folder was mounted.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We can just do it like how we do it on site,

revision = "alembic revision --autogenerate -m"
docker-revision = "docker-compose exec web alembic revision --autogenerate -m"

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.

Yea, this is purely for a docker setup. I would expect most people developing this to be running in docker, due to the psql need. Rather than running their own psql server.

So I'd suggest keeping this as the default and then having local-revision as the alternative for people who want to run psql locally.

Comment thread api/core/database/models/api/bot/user.py
Copy link
Copy Markdown

@doublevcodes doublevcodes left a comment

Choose a reason for hiding this comment

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

lgtm:tm:

Copy link
Copy Markdown
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

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

Approved by the Python Discord DevOps:tm: Team:tm:

@ChrisLovering ChrisLovering merged commit 530a62f into main Nov 15, 2021
@ChrisLovering ChrisLovering deleted the async-sqlalch branch November 15, 2021 19:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants