From c85e11aa48c7fb16eb0c07e941198330e1aa9065 Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Mon, 24 Jun 2024 14:58:47 -0700 Subject: [PATCH 1/8] Adding linting to migrations --- .github/workflows/lint-migrations.yml | 73 +++++++++++++++++++ src/dispatch/cli.py | 9 ++- src/dispatch/database/revisions/core/env.py | 38 +++++++++- src/dispatch/database/revisions/tenant/env.py | 39 +++++++++- 4 files changed, 154 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/lint-migrations.yml diff --git a/.github/workflows/lint-migrations.yml b/.github/workflows/lint-migrations.yml new file mode 100644 index 000000000000..cbe9abc6a719 --- /dev/null +++ b/.github/workflows/lint-migrations.yml @@ -0,0 +1,73 @@ +name: Eugene CI check + +env: + EUGENE_VERSION: "0.6.1" + +permissions: + contents: read + pull-requests: write + +jobs: + lint-migration: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Set up Python 3.11 + uses: actions/setup-python@v2 + with: + python-version: 3.11.2 + - uses: actions/cache@v2 + with: + path: ~/.cache/pip + key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} + restore-keys: | + ${{ runner.os }}-pip- + - name: Install python dependencies + run: | + export DISPATCH_LIGHT_BUILD=1 + python -m pip install --upgrade pip + pip install -e ".[dev]" + - name: Download eugene + run: | + curl -L https://github.com/kaaveland/eugene/releases/download/$EUGENE_VERSION/eugene-x86_64-unknown-linux-musl -o eugene + chmod +x eugene + - name: Generate sql files + run: | + # Navigate to the git repository root, if not already there + cd "$(git rev-parse --show-toplevel)" || exit + + # Perform git diff to check for changes in the specified directory + changed_files=$(git diff --name-only -- src/dispatch/database/revisions/tenant/versions) + + # Filter for newly added files (*.py) + new_files=$(echo "$changed_files" | grep -E "^src/dispatch/database/revisions/tenant/versions/2024-06-12_.*\.py$") + + # Check if there is exactly one new file and handle errors for multiple or no files + if [[ $(echo "$new_files" | wc -l) -ne 1 ]]; then + echo "Error: There should be exactly one new revision file. Found: $(echo "$new_files" | wc -l)" + exit 1 + fi + + new_file_path=$(echo "$new_files" | head -n 1) + new_revision_id=$(basename "$new_file_path" .py | cut -d'_' -f2) + + # Get the most recent revision ID excluding the newly added one + old_file=$(git ls-files src/dispatch/database/revisions/tenant/versions | grep -oP '2024-06-12_\K[^.]+(?=\.py)' | grep -v "$new_revision_id" | sort -rV | head -n 1) + + if [[ -z "$old_file" ]]; then + echo "Error: No old revision ID found" + exit 1 + fi + + old_revision_id=$(echo "$old_file") + dispatch database upgrade --revision-type tenant --revision="${old_revision_id}:${new_revision_id}" --sql + - name: Lint files + run: ./eugene lint --git-diff origin/main migration-scripts -f md --accept-failures > lint.md + - name: Post Comment + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + COMMENT=$(cat lint.md) + gh pr comment ${{ github.event.pull_request.number }} --body "$COMMENT" diff --git a/src/dispatch/cli.py b/src/dispatch/cli.py index 02551ba34115..5361700e5043 100644 --- a/src/dispatch/cli.py +++ b/src/dispatch/cli.py @@ -396,6 +396,8 @@ def drop_database(yes): def upgrade_database(tag, sql, revision, revision_type): """Upgrades database schema to newest version.""" import sqlalchemy + from contextlib import redirect_stdout + from alembic import command as alembic_command from alembic.config import Config as AlembicConfig from sqlalchemy import inspect @@ -426,7 +428,12 @@ def upgrade_database(tag, sql, revision, revision_type): path = config.ALEMBIC_TENANT_REVISION_PATH alembic_cfg.set_main_option("script_location", path) - alembic_command.upgrade(alembic_cfg, revision, sql=sql, tag=tag) + if sql: + with open("alembic_output.sql", "w") as sql_file: + with redirect_stdout(sql_file): + alembic_command.upgrade(alembic_cfg, revision, sql=sql, tag=tag) + else: + alembic_command.upgrade(alembic_cfg, revision, sql=sql, tag=tag) else: for path in [config.ALEMBIC_CORE_REVISION_PATH, config.ALEMBIC_TENANT_REVISION_PATH]: alembic_cfg.set_main_option("script_location", path) diff --git a/src/dispatch/database/revisions/core/env.py b/src/dispatch/database/revisions/core/env.py index 0e6967292ec3..c001ddfbf068 100644 --- a/src/dispatch/database/revisions/core/env.py +++ b/src/dispatch/database/revisions/core/env.py @@ -45,7 +45,10 @@ def process_revision_directives(context, revision, directives): log.info("No changes found skipping revision creation.") connectable = engine_from_config( - config.get_section(config.config_ini_section), prefix="sqlalchemy.", poolclass=pool.NullPool + config.get_section(config.config_ini_section), + prefix="sqlalchemy.", + poolclass=pool.NullPool, + connect_args={'options': '-c lock_timeout=4000 -c statement_timeout=5000'} ) log.info("Migrating dispatch core schema...") @@ -58,6 +61,7 @@ def process_revision_directives(context, revision, directives): target_metadata=target_metadata, include_schemas=True, include_object=include_object, + transaction_per_migration=True, process_revision_directives=process_revision_directives, ) @@ -65,7 +69,37 @@ def process_revision_directives(context, revision, directives): context.run_migrations() +def run_migrations_offline(): + """Run migrations in 'offline' mode. + + This configures the context with just a URL + and not an Engine, though an Engine is acceptable + here as well. By skipping the Engine creation + we don't even need a DBAPI to be available. + + Calls to context.execute() here emit the given string to the + script output. + + """ + + # Get the URL from the config + url = config.get_main_option("sqlalchemy.url") + + # Set the migration options + context.configure( + url=url, + target_metadata=target_metadata, + include_schemas=True, + include_object=include_object, + literal_binds=True, # Binds parameters with their string values + ) + + # Start a transaction and run migrations + with context.begin_transaction(): + context.run_migrations() + + if context.is_offline_mode(): - log.info("Can't run migrations offline") + run_migrations_offline() else: run_migrations_online() diff --git a/src/dispatch/database/revisions/tenant/env.py b/src/dispatch/database/revisions/tenant/env.py index 0aba8c3ee437..8e3d98a96cad 100644 --- a/src/dispatch/database/revisions/tenant/env.py +++ b/src/dispatch/database/revisions/tenant/env.py @@ -1,3 +1,4 @@ +import os from alembic import context from sqlalchemy import engine_from_config, pool, inspect @@ -51,7 +52,10 @@ def process_revision_directives(context, revision, directives): log.info("No changes found skipping revision creation.") connectable = engine_from_config( - config.get_section(config.config_ini_section), prefix="sqlalchemy.", poolclass=pool.NullPool + config.get_section(config.config_ini_section), + prefix="sqlalchemy.", + poolclass=pool.NullPool, + connect_args={'options': '-c lock_timeout=4000 -c statement_timeout=5000'} ) with connectable.connect() as connection: @@ -66,6 +70,7 @@ def process_revision_directives(context, revision, directives): target_metadata=target_metadata, include_schemas=True, include_object=include_object, + transaction_per_migration=True, process_revision_directives=process_revision_directives, ) @@ -77,7 +82,37 @@ def process_revision_directives(context, revision, directives): break +def run_migrations_offline(): + """Run migrations in 'offline' mode. + + This configures the context with just a URL + and not an Engine, though an Engine is acceptable + here as well. By skipping the Engine creation + we don't even need a DBAPI to be available. + + Calls to context.execute() here emit the given string to the + script output. + + """ + + # Get the URL from the config + url = config.get_main_option("sqlalchemy.url") + + # Set the migration options + context.configure( + url=url, + target_metadata=target_metadata, + include_schemas=True, + include_object=include_object, + literal_binds=True, # Binds parameters with their string values + ) + + # Start a transaction and run migrations + with context.begin_transaction(): + context.run_migrations() + + if context.is_offline_mode(): - log.info("Can't run migrations offline") + run_migrations_offline() else: run_migrations_online() From fafd35d0a32315edc565bb8a001fdafdcce68ffe Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Mon, 24 Jun 2024 15:34:01 -0700 Subject: [PATCH 2/8] Making it more reliable --- .github/workflows/lint-migrations.yml | 35 +++---------------- .github/workflows/playwright.yml | 2 +- .github/workflows/python.yml | 4 +-- src/dispatch/database/revisions/core/env.py | 8 ++++- src/dispatch/database/revisions/tenant/env.py | 9 +++-- 5 files changed, 21 insertions(+), 37 deletions(-) diff --git a/.github/workflows/lint-migrations.yml b/.github/workflows/lint-migrations.yml index cbe9abc6a719..79d6f53daa5b 100644 --- a/.github/workflows/lint-migrations.yml +++ b/.github/workflows/lint-migrations.yml @@ -15,10 +15,10 @@ jobs: with: fetch-depth: 0 - name: Set up Python 3.11 - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: 3.11.2 - - uses: actions/cache@v2 + - uses: actions/cache@v4 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} @@ -35,36 +35,9 @@ jobs: chmod +x eugene - name: Generate sql files run: | - # Navigate to the git repository root, if not already there - cd "$(git rev-parse --show-toplevel)" || exit - - # Perform git diff to check for changes in the specified directory - changed_files=$(git diff --name-only -- src/dispatch/database/revisions/tenant/versions) - - # Filter for newly added files (*.py) - new_files=$(echo "$changed_files" | grep -E "^src/dispatch/database/revisions/tenant/versions/2024-06-12_.*\.py$") - - # Check if there is exactly one new file and handle errors for multiple or no files - if [[ $(echo "$new_files" | wc -l) -ne 1 ]]; then - echo "Error: There should be exactly one new revision file. Found: $(echo "$new_files" | wc -l)" - exit 1 - fi - - new_file_path=$(echo "$new_files" | head -n 1) - new_revision_id=$(basename "$new_file_path" .py | cut -d'_' -f2) - - # Get the most recent revision ID excluding the newly added one - old_file=$(git ls-files src/dispatch/database/revisions/tenant/versions | grep -oP '2024-06-12_\K[^.]+(?=\.py)' | grep -v "$new_revision_id" | sort -rV | head -n 1) - - if [[ -z "$old_file" ]]; then - echo "Error: No old revision ID found" - exit 1 - fi - - old_revision_id=$(echo "$old_file") - dispatch database upgrade --revision-type tenant --revision="${old_revision_id}:${new_revision_id}" --sql + dispatch database upgrade --revision-type tenant --sql - name: Lint files - run: ./eugene lint --git-diff origin/main migration-scripts -f md --accept-failures > lint.md + run: ./eugene lint alembic_output.sql -f md --accept-failures > lint.md - name: Post Comment env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml index 1a7897144757..bcf23b462174 100644 --- a/.github/workflows/playwright.yml +++ b/.github/workflows/playwright.yml @@ -31,7 +31,7 @@ jobs: - name: Check out Git repository uses: actions/checkout@v3 - name: Set up Python 3.11 - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: 3.11 - uses: actions/setup-node@v3 diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index ed9a547ebc82..2f652f531b7b 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -25,10 +25,10 @@ jobs: - name: Check out Git repository uses: actions/checkout@v2 - name: Set up Python 3.11 - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: 3.11.2 - - uses: actions/cache@v2 + - uses: actions/cache@v4 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} diff --git a/src/dispatch/database/revisions/core/env.py b/src/dispatch/database/revisions/core/env.py index c001ddfbf068..364a1489ed63 100644 --- a/src/dispatch/database/revisions/core/env.py +++ b/src/dispatch/database/revisions/core/env.py @@ -1,4 +1,4 @@ -from alembic import context +from alembic import context, script from sqlalchemy import engine_from_config, pool from dispatch.logging import logging @@ -85,6 +85,10 @@ def run_migrations_offline(): # Get the URL from the config url = config.get_main_option("sqlalchemy.url") + alembic_script = script.ScriptDirectory.from_config(config) + + _, previous = list(alembic_script.walk_revisions(base='base', head='heads'))[:2] + # Set the migration options context.configure( url=url, @@ -92,6 +96,7 @@ def run_migrations_offline(): include_schemas=True, include_object=include_object, literal_binds=True, # Binds parameters with their string values + starting_rev=previous.revision, ) # Start a transaction and run migrations @@ -99,6 +104,7 @@ def run_migrations_offline(): context.run_migrations() + if context.is_offline_mode(): run_migrations_offline() else: diff --git a/src/dispatch/database/revisions/tenant/env.py b/src/dispatch/database/revisions/tenant/env.py index 8e3d98a96cad..d09b83cd5077 100644 --- a/src/dispatch/database/revisions/tenant/env.py +++ b/src/dispatch/database/revisions/tenant/env.py @@ -1,5 +1,5 @@ -import os -from alembic import context +from alembic import context, script + from sqlalchemy import engine_from_config, pool, inspect @@ -98,6 +98,10 @@ def run_migrations_offline(): # Get the URL from the config url = config.get_main_option("sqlalchemy.url") + alembic_script = script.ScriptDirectory.from_config(config) + + _, previous = list(alembic_script.walk_revisions(base='base', head='heads'))[:2] + # Set the migration options context.configure( url=url, @@ -105,6 +109,7 @@ def run_migrations_offline(): include_schemas=True, include_object=include_object, literal_binds=True, # Binds parameters with their string values + starting_rev=previous.revision, ) # Start a transaction and run migrations From add70fb26b69cf234fdebef2b0b5c96811990cfe Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Mon, 24 Jun 2024 15:41:58 -0700 Subject: [PATCH 3/8] Adding trigger --- .github/workflows/lint-migrations.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/lint-migrations.yml b/.github/workflows/lint-migrations.yml index 79d6f53daa5b..cecf0d0e83b9 100644 --- a/.github/workflows/lint-migrations.yml +++ b/.github/workflows/lint-migrations.yml @@ -1,5 +1,7 @@ name: Eugene CI check +on: pull_request + env: EUGENE_VERSION: "0.6.1" From 93bd46d7619b98b3d0b2468d13b6cf1fd51ff624 Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Mon, 24 Jun 2024 15:46:48 -0700 Subject: [PATCH 4/8] Adding dummy key --- .github/workflows/lint-migrations.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/lint-migrations.yml b/.github/workflows/lint-migrations.yml index cecf0d0e83b9..890c1cf2395b 100644 --- a/.github/workflows/lint-migrations.yml +++ b/.github/workflows/lint-migrations.yml @@ -37,6 +37,7 @@ jobs: chmod +x eugene - name: Generate sql files run: | + export DISPATCH_ENCRYPTION_KEY=supersecret dispatch database upgrade --revision-type tenant --sql - name: Lint files run: ./eugene lint alembic_output.sql -f md --accept-failures > lint.md From feacf6b979edaba62bf56332b168ec869d1a499a Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Mon, 24 Jun 2024 15:50:18 -0700 Subject: [PATCH 5/8] More dummy vars --- .github/workflows/lint-migrations.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/lint-migrations.yml b/.github/workflows/lint-migrations.yml index 890c1cf2395b..ea35a15b82e6 100644 --- a/.github/workflows/lint-migrations.yml +++ b/.github/workflows/lint-migrations.yml @@ -37,8 +37,13 @@ jobs: chmod +x eugene - name: Generate sql files run: | - export DISPATCH_ENCRYPTION_KEY=supersecret - dispatch database upgrade --revision-type tenant --sql + export LOG_LEVEL="ERROR" + export STATIC_DIR="" + export DATABASE_HOSTNAME="localhost" + export DATABASE_CREDENTIALS="dispatch:dispatch" + export DISPATCH_ENCRYPTION_KEY="NJHDWDJ3PbHT8h" + export DISPATCH_JWT_SECRET="foo" + dispatch database upgrade --revision-type tenant --sql - name: Lint files run: ./eugene lint alembic_output.sql -f md --accept-failures > lint.md - name: Post Comment From bfc69247efdb4d4e8b9fed6304722830dca78368 Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Mon, 24 Jun 2024 15:58:25 -0700 Subject: [PATCH 6/8] Removing need for live database --- src/dispatch/cli.py | 54 ++++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/dispatch/cli.py b/src/dispatch/cli.py index 5361700e5043..75d946534477 100644 --- a/src/dispatch/cli.py +++ b/src/dispatch/cli.py @@ -407,37 +407,41 @@ def upgrade_database(tag, sql, revision, revision_type): from .database.manage import init_database alembic_cfg = AlembicConfig(config.ALEMBIC_INI_PATH) + path = None + if revision_type: + if revision_type == "core": + path = config.ALEMBIC_CORE_REVISION_PATH - if not database_exists(str(config.SQLALCHEMY_DATABASE_URI)): - click.secho("Found no database to upgrade, initializing new database...") - init_database(engine) - else: - conn = engine.connect() - - # detect if we need to convert to a multi-tenant schema structure - schema_names = inspect(engine).get_schema_names() - if "dispatch_core" not in schema_names: - click.secho("Detected single tenant database, converting to multi-tenant...") - conn.execute(sqlalchemy.text(open(config.ALEMBIC_MULTI_TENANT_MIGRATION_PATH).read())) - - if revision_type: - if revision_type == "core": - path = config.ALEMBIC_CORE_REVISION_PATH + elif revision_type == "tenant": + path = config.ALEMBIC_TENANT_REVISION_PATH - elif revision_type == "tenant": - path = config.ALEMBIC_TENANT_REVISION_PATH + alembic_cfg.set_main_option("script_location", path) - alembic_cfg.set_main_option("script_location", path) - if sql: - with open("alembic_output.sql", "w") as sql_file: - with redirect_stdout(sql_file): - alembic_command.upgrade(alembic_cfg, revision, sql=sql, tag=tag) - else: + # doesn't support core and tenant at the same time + if sql: + with open("alembic_output.sql", "w") as sql_file: + with redirect_stdout(sql_file): alembic_command.upgrade(alembic_cfg, revision, sql=sql, tag=tag) + + else: + if not database_exists(str(config.SQLALCHEMY_DATABASE_URI)): + click.secho("Found no database to upgrade, initializing new database...") + init_database(engine) else: - for path in [config.ALEMBIC_CORE_REVISION_PATH, config.ALEMBIC_TENANT_REVISION_PATH]: - alembic_cfg.set_main_option("script_location", path) + conn = engine.connect() + + # detect if we need to convert to a multi-tenant schema structure + schema_names = inspect(engine).get_schema_names() + if "dispatch_core" not in schema_names: + click.secho("Detected single tenant database, converting to multi-tenant...") + conn.execute(sqlalchemy.text(open(config.ALEMBIC_MULTI_TENANT_MIGRATION_PATH).read())) + + if path: alembic_command.upgrade(alembic_cfg, revision, sql=sql, tag=tag) + else: + for path in [config.ALEMBIC_CORE_REVISION_PATH, config.ALEMBIC_TENANT_REVISION_PATH]: + alembic_cfg.set_main_option("script_location", path) + alembic_command.upgrade(alembic_cfg, revision, sql=sql, tag=tag) click.secho("Success.", fg="green") From 6ec89df92dacc48d2921b65b5dfeacba8db5d331 Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Tue, 25 Jun 2024 08:33:58 -0700 Subject: [PATCH 7/8] Timeouts handled globally --- .github/workflows/lint-migrations.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint-migrations.yml b/.github/workflows/lint-migrations.yml index ea35a15b82e6..f2e9bc0f25bb 100644 --- a/.github/workflows/lint-migrations.yml +++ b/.github/workflows/lint-migrations.yml @@ -45,7 +45,7 @@ jobs: export DISPATCH_JWT_SECRET="foo" dispatch database upgrade --revision-type tenant --sql - name: Lint files - run: ./eugene lint alembic_output.sql -f md --accept-failures > lint.md + run: ./eugene lint --ignore E9 alembic_output.sql -f md --accept-failures > lint.md - name: Post Comment env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} From 84fa0586d2578057f707277dee6c8aed4a0fa973 Mon Sep 17 00:00:00 2001 From: Kevin Glisson Date: Tue, 25 Jun 2024 08:56:22 -0700 Subject: [PATCH 8/8] Only run if there are migrations --- .github/workflows/lint-migrations.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/lint-migrations.yml b/.github/workflows/lint-migrations.yml index f2e9bc0f25bb..20d4acf9df54 100644 --- a/.github/workflows/lint-migrations.yml +++ b/.github/workflows/lint-migrations.yml @@ -43,6 +43,12 @@ jobs: export DATABASE_CREDENTIALS="dispatch:dispatch" export DISPATCH_ENCRYPTION_KEY="NJHDWDJ3PbHT8h" export DISPATCH_JWT_SECRET="foo" + + changed_files=$(git diff --name-only -- src/dispatch/database/revisions/tenant/versions) + if [ -z "$changed_files" ]; then + echo "No migration files changed, skipping linting" + exit 0 + fi dispatch database upgrade --revision-type tenant --sql - name: Lint files run: ./eugene lint --ignore E9 alembic_output.sql -f md --accept-failures > lint.md