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

Move dev DB schema creation and stamping to a separate process #1123

Merged
merged 1 commit into from
Jul 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .cookiecutter/includes/conf/supervisord-dev.conf.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
{
"programs": {
"make_db": {
"command": "bin/make_db",
"startsecs": "0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default if bin/make_db exits in under a second (which it easily can do) Supervisor will consider this a crash regardless of the exit code and will restart the process. Setting startsecs to 0 tells Supervisor not to consider this a crash and not to restart the process (unless the exit code is non-zero)

},
"web": {
"command": "gunicorn -c conf/gunicorn/dev.conf.py --paste conf/development.ini"
},
Expand Down
36 changes: 33 additions & 3 deletions bin/make_db
Original file line number Diff line number Diff line change
@@ -1,5 +1,35 @@
"""Initialize the dev environment's DB."""
#!/usr/bin/env python3
import sys
"""Initialize the dev environment's DB."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The #! needs to be above the docstring or it doesn't work! 🤦

bin/make_db actually comes from the cookiecutter, so I'll need to move these changes into the cookiecutter

import alembic.command
import alembic.config
from pyramid.paster import bootstrap
bootstrap("conf/development.ini")
from sqlalchemy import text
from sqlalchemy.exc import ProgrammingError

from via.db import Base, create_engine


def is_stamped(engine) -> bool:
"""Return True if the DB is stamped with an Alembic revision ID."""
with engine.connect() as connection:
try:
if connection.execute(text("select * from alembic_version")).first():
return True
except ProgrammingError:
pass

return False


def main():
with bootstrap("conf/development.ini") as env:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with is the recommended way to use bootstrap() in the Pyramid docs, make_db wasn't doing this before

settings = env["registry"].settings
engine = create_engine(settings)

if not is_stamped(engine):
Base.metadata.create_all(engine)
alembic.command.stamp(alembic.config.Config("conf/alembic.ini"), "head")
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned on the other PR I reckon either here or in the Makefile
we should run the migrations as part of the make db process

https://github.com/hypothesis/lms/blob/main/Makefile#L18

I reckon with that we'll get a "it just works" experience for dev that are not making changes here often and just want to have the service running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Via does run the migrations in Makefile. I think make db has always done this for all our apps?

It may be a bit confusing that there's a bin/make_db script, but calling that script is only one of two commands that make db runs. May want to move the alembic uprade command into the make_db script somehow



if __name__ == "__main__":
main()
8 changes: 8 additions & 0 deletions conf/supervisord-dev.conf
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@
nodaemon=true
silent=true

[program:make_db]
command=bin/make_db
stdout_events_enabled=true
stderr_events_enabled=true
stopsignal=KILL
stopasgroup=true
startsecs=0

[program:web]
command=gunicorn -c conf/gunicorn/dev.conf.py --paste conf/development.ini
stdout_events_enabled=true
Expand Down
29 changes: 6 additions & 23 deletions via/db.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import alembic.command
import alembic.config
import sqlalchemy
import zope.sqlalchemy
from sqlalchemy import MetaData, create_engine, text
from sqlalchemy.exc import ProgrammingError
from sqlalchemy import MetaData
from sqlalchemy.orm import DeclarativeBase, MappedAsDataclass, Session


Expand All @@ -18,23 +16,13 @@ class Base(MappedAsDataclass, DeclarativeBase):
)


def stamp(engine): # pragma: no cover
"""If the DB isn't already stamped then stamp it with the latest revision."""
stamped = False

with engine.connect() as connection:
try:
if connection.execute(text("select * from alembic_version")).first():
stamped = True
except ProgrammingError:
pass

if not stamped:
alembic.command.stamp(alembic.config.Config("conf/alembic.ini"), "head")
def create_engine(settings): # pragma: no cover
"""Return a SQLAlchemy engine."""
return sqlalchemy.create_engine(settings["database_url"])


def includeme(config): # pragma: no cover
engine = create_engine(config.registry.settings["database_url"])
engine = create_engine(config.registry.settings)

def db_session(request):
"""Return the SQLAlchemy session for the given request."""
Expand All @@ -47,11 +35,6 @@ def db_session(request):

return session

if config.registry.settings.get("dev"):
# Create the database tables if they don't already exist.
Base.metadata.create_all(engine)
stamp(engine)
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly not having dev specific stuff in here seems like a win to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


# Make the SQLAlchemy session available as `request.db`.
# `reify=True` means it'll create only one session per request.
config.add_request_method(db_session, name="db", reify=True)