Feature/move fixtures to migrations #73

mwestphall · 2024-07-25T20:31:23Z

Create a database migration for importing the baseline schema from the most recent prod dump
Move all "fixture" database migrations into the migrations cli subsystem system
Give the ability to specify whether a migration depends on one or more other migrations having completed
Pull up a lot of the logic from individual subclasses of Migration into the main Migration class to allow for more concise definition of new migrations
Add a docker-compose for spinning up a local postgis instance with appropriate extensions installed

…ut not prod

…able to apply or has already applied

…a (a bit hacky)

davenquinn

This looks great! A few changes to take a look at, but it looks nearly ready to merge.

davenquinn · 2024-07-25T20:49:37Z

cli/macrostrat/cli/database/migrations/base.py

+    CAN_APPLY = "can_apply"
+
+    # The postconditions for this migration are met, so it doesn't need to be applied
+    APPLIED = "applied"


Would there be any value in having a state UNNECESSARY which would mean that the postconditions are met but the preconditions are not? This could be valuable for migrations that are to recover from invalid/legacy database states of some sort.

Put another way, is there value to having the difference between CANT_APPLY and not APPLIED (needs running but can't be right now) and CANT_APPLY and APPLIED (does not need running since database is already in a valid state)?

I don't think the current implementation of checking for whether a migration is applied is sophisticated enough to handle that use case, since it just checks the current state of tables in the database and assigns one of the 3 labels based on that state. We might be able to get at this by looking further down the dependency graph, eg. checking whether any dependents of a CANT_APPLY migration are in an APPLIED state.

davenquinn · 2024-07-25T20:50:54Z

cli/macrostrat/cli/database/migrations/__init__.py

-        MacrostratCoreMigration,
-    ]
+    # Find all subclasses of Migration among imported modules
+    migrations = Migration.__subclasses__() 


I am into this but maybe it's a bit too implicit? Should there be a register_migration method instead?

this could be wrapped into a MigrationsManager class

this might allow us to break up migrations by subsystem, if desired

Given the DROP SCHEMA macrostrat command attempted in the MacrostratCoreMigration, I think it'll be necessary to disable that migration for now at least on the main branch. Don't want to have data-destroying migrations quite yet until we're satisfied that the MariaDB -> Postgres migration into the Macrostrat schema is functional. Then I think we can rewrite that baseline migration quite substantially.

davenquinn · 2024-07-25T20:55:35Z

cli/macrostrat/cli/database/migrations/__init__.py

-    for cls in migrations:
-        # Initialize migration
-        _migration = cls()
+    for _migration in instances:


One pattern we could eventually add here, which I have successfully used in the past, is to have a "dry run" migration automatically run first, based on a schema-only clone of the database.

davenquinn · 2024-07-25T20:56:24Z

cli/macrostrat/cli/database/_legacy.py

+    if db is not None:
+        db.session.flush()
+        db.session.close()
+    db = Database(PG_DATABASE)


We should add this method to the macrostrat.database module.

davenquinn · 2024-07-25T21:02:10Z

cli/macrostrat/cli/database/migrations/api_v3/__init__.py

+    depends_on = ['map-source-slug']
+
+    # Confirm that the tables created by the API v3 migrations are present
+    postconditions = [


Postcondition checks are a fantastic addition. Looks like you went for higher-order functions simply to enhance composability, yeah?

davenquinn · 2024-07-25T21:02:51Z

cli/macrostrat/cli/database/migrations/api_v3/__init__.py

+
+    # Confirm that the tables created by the API v3 migrations are present
+    postconditions = [
+        exists("storage","object_group","object"),


might want to use a pattern like tables_exist("object_group", "object", schema="storage") for enhanced readability.

davenquinn · 2024-07-25T21:05:47Z

cli/macrostrat/cli/database/migrations/column_builder/__init__.py

+
+from psycopg2.sql import Identifier
+
+MATCHES_SLUG_SQL = """


this doesn't look used here?

davenquinn · 2024-07-25T21:09:02Z

docker-compose.yaml

Perhaps this could be merged with local-root/docker-compose.yaml? This is what I use for a local copy of Macrostrat currently. Could perhaps rename this directory for clarity.

davenquinn · 2024-07-25T21:11:21Z

db-local.Dockerfile

@@ -0,0 +1,5 @@
+FROM postgis/postgis:15-3.4
+RUN apt-get update && apt-get install -y --no-install-recommends \
+  postgresql-$PG_MAJOR-pgaudit


This is great! I've been meaning to get pgaudit working in my local environment.

I've used imresamu/postgis:15-3.4 while postgis/docker-postgis#216 is open, to help those of use on ARM machines.

davenquinn · 2024-07-25T22:16:59Z

cli/macrostrat/cli/database/migrations/__init__.py

-        MacrostratCoreMigration,
-    ]
+    # Find all subclasses of Migration among imported modules
+    migrations = Migration.__subclasses__() 


Given the DROP SCHEMA macrostrat command attempted in the MacrostratCoreMigration, I think it'll be necessary to disable that migration for now at least on the main branch. Don't want to have data-destroying migrations quite yet until we're satisfied that the MariaDB -> Postgres migration into the Macrostrat schema is functional. Then I think we can rewrite that baseline migration quite substantially.

mwestphall added 14 commits July 23, 2024 12:10

add migration for baseline prod db state

1ce7654

add Dockerfile for local db

35a028e

add migration for maps.sources slug column, primary_table renames

1550f42

fix issue with unpacking tuple

1726b2f

Add missing primary key to sources

ffc1108

Add migration to define tables present macrostrat schema of staging b…

fde1f41

…ut not prod

Add sort order to migrations, move most fixtures to migrations

61692d8

start refactoring 'should apply' to determine whether a migration is …

814d150

…able to apply or has already applied

replace custom 'should_apply' functions with list of existance checks

5a72a22

reset the db connection between migrations to refresh inspector schem…

b4f102b

…a (a bit hacky)

clean up documentation of Migration class

fe2f66b

move remaining fixtures to migrations

d861c16

fix names

56d90cd

Merge branch 'main' into feature/move-fixtures-to-migrations

99c8277

mwestphall requested review from davenquinn and brianaydemir July 25, 2024 20:31

fix uncommitted merge conflict resolution

c13739a

davenquinn reviewed Jul 25, 2024

View reviewed changes

merge migrations testing docker definitions with existing docker-compose

2af13e5

davenquinn requested changes Jul 25, 2024

View reviewed changes

mwestphall force-pushed the feature/move-fixtures-to-migrations branch from 8dcccc2 to 2af13e5 Compare July 29, 2024 16:12

mwestphall added 3 commits July 29, 2024 11:16

Add a couple missing dependencies

d09cf62

Add migration flag to indicate a migration alters existing data

0501fd7

name macrostrat schema foreign keys to prevent duplicate creation

7b1a830

davenquinn merged commit aeea2d2 into UW-Macrostrat:main Jul 29, 2024

davenquinn mentioned this pull request Jul 30, 2024

SQL fixtures needed for tileserver operation UW-Macrostrat/tileserver#38

Open

davenquinn mentioned this pull request Aug 8, 2024

Command to migrate MariaDB database #61

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/move fixtures to migrations #73

Feature/move fixtures to migrations #73

mwestphall commented Jul 25, 2024

davenquinn left a comment

davenquinn Jul 25, 2024

mwestphall Jul 25, 2024

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024 •

edited

Loading

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024

davenquinn Jul 25, 2024 •

edited

Loading

Feature/move fixtures to migrations #73

Feature/move fixtures to migrations #73

Conversation

mwestphall commented Jul 25, 2024

davenquinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davenquinn Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davenquinn Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

davenquinn Jul 25, 2024 •

edited

Loading

davenquinn Jul 25, 2024 •

edited

Loading