Adding linting to migrations

[kevgliss]

This PR adds a new type of linting for our alembic migration files. It dumps all migrations to .sql files and then runs a subset of linting rules that looks for problematic locks and other issues. Currently, it only attempts to look at the last added revision, as we generally only have one migration per pr; I think this is probably reasonable. If we find value we can extend this a bit.

Documentation reference: https://kaveland.no/eugene/introduction.html

Additionally, I have added several improvements to the actual migrations themselves based on the advice here:
https://squawkhq.com/docs/web-frameworks/#alembic-and-sqlalchemy. This should help prevent migrations from locking up the database if we accidentally introduce a change that requires exclusivity.

[github-actions]

Eugene 🔒 lint report of alembic_output.sql

This is a human readable SQL script safety report generated by eugene.
Keep in mind that lints can be ignored by adding a -- eugene: ignore E123 comment to the SQL statement
or by passing --ignore E123 on the command line.

The migration script did not pass all the checks ❌

Statement number 1

SQL

BEGIN

No checks matched for this statement. ✅

Statement number 2

SQL

-- Running upgrade a836d4850a75 -> 4286dcce0a2d

ALTER TABLE incident_type ADD COLUMN channel_description VARCHAR

Lints

Taking dangerous lock without timeout

ID: E9

A lock that would block many common operations was taken without a timeout. This can block all other operations on the table indefinitely if any other transaction holds a conflicting lock while idle in transaction or active. A safer way is: Run SET LOCAL lock_timeout = '2s'; before the statement and retry the migration if necessary.

Statement takes lock on public.incident_type, but does not set a lock timeout.

Statement number 3

SQL

ALTER TABLE incident_type ADD COLUMN description_service_id INTEGER

Lints

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

Taking dangerous lock without timeout

ID: E9

A lock that would block many common operations was taken without a timeout. This can block all other operations on the table indefinitely if any other transaction holds a conflicting lock while idle in transaction or active. A safer way is: Run SET LOCAL lock_timeout = '2s'; before the statement and retry the migration if necessary.

Statement takes lock on public.incident_type, but does not set a lock timeout.

Multiple ALTER TABLE statements where one will do

ID: W12

Multiple ALTER TABLE statements targets the same table. If the statements require table scans, there will be more scans than necessary. A safer way is: Combine the statements into one, separating the action with commas.

Multiple ALTER TABLE statements on public.incident_type. Combine them into a single statement to avoid scanning the table multiple times..

Statement number 4

SQL

ALTER TABLE incident_type ADD CONSTRAINT description_service_id_fkey FOREIGN KEY(description_service_id) REFERENCES service (id)

Lints

Validating table with a new constraint

ID: E1

A new constraint was added and it is already VALID. This blocks all table access until all rows are validated. A safer way is: Add the constraint as NOT VALID and validate it with ALTER TABLE ... VALIDATE CONSTRAINT later.

Statement takes AccessExclusiveLock on public.incident_type, blocking reads until constraint description_service_id_fkey is validated.

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

Taking dangerous lock without timeout

ID: E9

A lock that would block many common operations was taken without a timeout. This can block all other operations on the table indefinitely if any other transaction holds a conflicting lock while idle in transaction or active. A safer way is: Run SET LOCAL lock_timeout = '2s'; before the statement and retry the migration if necessary.

Statement takes lock on public.incident_type, but does not set a lock timeout.

Multiple ALTER TABLE statements where one will do

ID: W12

Multiple ALTER TABLE statements targets the same table. If the statements require table scans, there will be more scans than necessary. A safer way is: Combine the statements into one, separating the action with commas.

Multiple ALTER TABLE statements on public.incident_type. Combine them into a single statement to avoid scanning the table multiple times..

Statement number 5

SQL

UPDATE alembic_version SET version_num='4286dcce0a2d' WHERE alembic_version.version_num = 'a836d4850a75'

Lints

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

Statement number 6

SQL

COMMIT

Lints

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

[github-actions]

Eugene 🔒 lint report of alembic_output.sql

This is a human readable SQL script safety report generated by eugene.
Keep in mind that lints can be ignored by adding a -- eugene: ignore E123 comment to the SQL statement
or by passing --ignore E123 on the command line.

The migration script did not pass all the checks ❌

Statement number 1

SQL

BEGIN

No checks matched for this statement. ✅

Statement number 2

SQL

-- Running upgrade a836d4850a75 -> 4286dcce0a2d

ALTER TABLE incident_type ADD COLUMN channel_description VARCHAR

No checks matched for this statement. ✅

Statement number 3

SQL

ALTER TABLE incident_type ADD COLUMN description_service_id INTEGER

Lints

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

Multiple ALTER TABLE statements where one will do

ID: W12

Multiple ALTER TABLE statements targets the same table. If the statements require table scans, there will be more scans than necessary. A safer way is: Combine the statements into one, separating the action with commas.

Multiple ALTER TABLE statements on public.incident_type. Combine them into a single statement to avoid scanning the table multiple times..

Statement number 4

SQL

ALTER TABLE incident_type ADD CONSTRAINT description_service_id_fkey FOREIGN KEY(description_service_id) REFERENCES service (id)

Lints

Validating table with a new constraint

ID: E1

A new constraint was added and it is already VALID. This blocks all table access until all rows are validated. A safer way is: Add the constraint as NOT VALID and validate it with ALTER TABLE ... VALIDATE CONSTRAINT later.

Statement takes AccessExclusiveLock on public.incident_type, blocking reads until constraint description_service_id_fkey is validated.

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

Multiple ALTER TABLE statements where one will do

ID: W12

Multiple ALTER TABLE statements targets the same table. If the statements require table scans, there will be more scans than necessary. A safer way is: Combine the statements into one, separating the action with commas.

Multiple ALTER TABLE statements on public.incident_type. Combine them into a single statement to avoid scanning the table multiple times..

Statement number 5

SQL

UPDATE alembic_version SET version_num='4286dcce0a2d' WHERE alembic_version.version_num = 'a836d4850a75'

Lints

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

Statement number 6

SQL

COMMIT

Lints

Running more statements after taking AccessExclusiveLock

ID: E4

A transaction that holds an AccessExclusiveLock started a new statement. This blocks all access to the table for the duration of this statement. A safer way is: Run this statement in a new transaction.

Running more statements after taking AccessExclusiveLock.

[whitdog47]

It looks like the playwright tests are failing since the path is still None and line set_main_option in line 418 requires a string.

[metroid-samus]

Suggested change

	alembic_cfg.set_main_option("script_location", path)
	alembic_cfg.set_main_option("script_location", path)

[github-actions]

This PR is stale, because it has been open for 45 days with no activity. Remove the stale label or comment, or this will be closed in 10 days.

[github-actions]

This PR was closed, because it has been stalled for 10 days with no activity.