search

Artur-Sulej / excellent_migrations

An Elixir tool for checking safety of database migrations.
MIT License
232 stars 25 forks source link

Danger detected incorrectly for repos that use `migration_lock: :pg_advisory_lock` #37

Closed dbernheisel closed 5 months ago

dbernheisel commented 8 months ago

My repo is configured to use the pg advisory lock strategy for locking migrations across nodes, which does not use a database transaction, and when I assure ExcellentMigrations that it's ok to not use the migration lock, and I'm creating an index concurrently, it still complains about not disabling the ddl transaction even though I have.

I traced it to this case statement that has a catchall clause to add the DDL Transaction danger, which seems incorrect.

Here's an example setup:

config :my_app, MyApp.Repo,
  migration_lock: :pg_advisory_lock
defmodule MyApp.Repo.Migrations.MyMigration do
  use Ecto.Migration
  @disable_ddl_transaction true

  # excellent_migrations:safety-assured-for-this-file index_concurrently_without_disable_migration_lock

  def change do
    create_if_not_exists index("foo", [:bar], concurrently: true)
  end
end
my_app ❯ mix credo
Checking 2682 source files (this might take a while) ...

  Warnings - please take a look
┃ 
┃ [W] ↗ Index concurrently without disable ddl transaction
┃       priv/repo/migrations/20240125215410_my_migration.exs:6 #(MyApp.Repo.Migrations.MyMigration.change)
Artur-Sulej commented 6 months ago

Hi @dbernheisel! Sorry for a delay in my response.

I was unable to reproduce the issue using the latest version. However, with version 0.1.6, I did observe the incorrect danger with your example migration. It seems that this PR has already fixed the problem: #24

Can you update excellent_migrations to the newest version (0.1.8) and let me know if it works fine for you?

dbernheisel commented 5 months ago

yep! Sorry for the noise. Indeed is fixed on 0.1.8