Page MenuHomePhabricator

Scaffold alembic
ClosedPublic

Authored by max on Nov 23 2019, 1:52 AM.

Details

Summary

Prep for the ability to migrate SQL schemata. Stacked on D1470, D1472.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

this one might be best to do in person walk through - not obvious what here is all machine generated artifacts vs edited / manually written

All of this is boilerplate, I've separated it into its own diff so as not to complicate review of all the manual stuff. We could maybe condense this by having our own library functions, like dagster.utils.sql.run_migrations_offline and dagster.utils.sql.run_migrations_online.

This is fine. More high level, can I get context as to why are we doing this? This is weird that we are saying, you have to use alembic if you use the dagster postgres library. Also why is run storage coupled so tightly to this lib? Is this lib purely internal? (Sorry I am catching up here)

Context is, we need some way to version and migrate the schemas for our storages. The dagster-postgres library contains postgres-backed storages. We are trying to use a single schema definition for all of our SQL-backed storages, so that we can have unified migration tooling. (Obviously if you write your own storage class, you don't need to worry about backwards compatibility if you don't need to worry about it.)

Got it. Essentially, clients who want to use our postgres backed solution, get this for free and need to use alembic. If they disagree they can implement their own thing (with relative ease since it's all configurable in DAGSTER_HOME. Cool!

This revision is now accepted and ready to land.Nov 25 2019, 8:18 PM

Ideally, they won't use alembic directly -- we will call alembic APIs from dagster instance migrate, which will call the upgrade() method on the storages. Would love your eyes on this setup in https://dagster.phacility.com/D1471

python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/alembic.ini
15

-N should go away given the new version of black we are on right?

python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/alembic.ini
15

hyup

This revision was automatically updated to reflect the committed changes.