Skip to content
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

feat: configure sqlalchemy and add alembic migrations #12

Closed
wants to merge 0 commits into from
Closed

feat: configure sqlalchemy and add alembic migrations #12

wants to merge 0 commits into from

Conversation

raf-nr
Copy link
Contributor

@raf-nr raf-nr commented Nov 18, 2023

Configure SQLAlchemy
Add alembic migrations
Add instructions in the makefile to start the database and run migrations

Copy link
Collaborator

@toadharvard toadharvard left a comment

Choose a reason for hiding this comment

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

CI падает
Коммиты названы как попало, последний вообще merge...
История должна быть линейной. Переделай коммиты так, чтобы они были атомарными.

pyproject.toml Outdated
@@ -14,6 +14,7 @@ sqlalchemy = "^2.0.22"
alembic = "^1.12.1"
celery = "^5.3.4"
python-dotenv = "^1.0.0"
psycopg2-binary = "^2.9.5"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Есть 3 версия и она поддерживается


settings = get_settings()

engine = create_engine(url=settings.postgres_dsn, echo=True)
Copy link
Collaborator

@toadharvard toadharvard Nov 19, 2023

Choose a reason for hiding this comment

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

он точно сможет создать engine от настройки не строки?
зачем echo=True в продакшене?

@@ -0,0 +1,3 @@
from sqlalchemy.orm import declarative_base

Copy link
Collaborator

Choose a reason for hiding this comment

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

Мне не нравится идея складывать основные объекты для работы с базой данных в db/postgres, потому что никакой другой базы, кроме этой быть не может.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Я бы сложил все, что нужно для работы с базой данных, сессии, миграции, зависимости и прочее в app/db
Модели же должны лежать в отдельных папках связанных с конкретными функциаональными зависимостями (т.е не в app/db)

# are written from script.py.mako
# output_encoding = utf-8

sqlalchemy.url = %(DB_URL)s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут не пользуются капсом, это настройка, а не env переменная

Makefile Outdated
@@ -1,5 +1,14 @@
.PHONY: env install-deps up worker app init lint test

include .env
Copy link
Collaborator

Choose a reason for hiding this comment

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

Эта строчка все ломает при чистой установке. В том числе и CI.

Makefile Outdated
include .env

args := $(wordlist 2, 100, $(MAKECMDGOALS))
ifndef args
Copy link
Collaborator

Choose a reason for hiding this comment

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

MESSAGE никогда не используется

Copy link
Collaborator

Choose a reason for hiding this comment

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

Зачем эта строка?

Makefile Outdated
@@ -16,6 +25,18 @@ up:
(trap 'docker compose -f dev-docker-compose.yaml down' INT; \
docker compose -f dev-docker-compose.yaml up --build --force-recreate --remove-orphans)

## Open database with docker-compose
open_db:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
open_db:
open-db:

Makefile Outdated
@@ -1,5 +1,14 @@
.PHONY: env install-deps up worker app init lint test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Сюда не добавил новые таргеты

@toadharvard
Copy link
Collaborator

Возможно стоит переложить alembic.ini в /settings
Потому что это фактически просто настройка для миграций. Но это опционально

@raf-nr raf-nr closed this Nov 19, 2023
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