feat: dev flag for generating migrations

[ken-kost]

#490

I think the fix here is actually to support a --dev flag for migrations, which uses numbered migrations like Dev1 and Dev2 etc. Then, when migrations are checked with mix ash.codegen --check, we error if there are dev migrations. When you run mix ash.codegen name we rollback all dev migrations, delete all dev snapshots and regenerate them fully.

For now I'm just adding _dev everywhere and I left the timestamp which gives it uniqueness.
For now just deleting _dev files when --dev is not provided.

Not sure if in the right direction, need feedback. Putting this into draft.

Contributor checklist

• Bug fixes include regression tests
• Features include unit/acceptance tests

[ken-kost]

To try out make some changes and run
mix ash_postgres.generate_migrations --dev
it will create snapshots and migrations with _dev ending.
mix ash_postgres.generate_migrations
will remove all migrations and snapshots with _dev ending and continue as usual.

[zachdaniel]

This is great! I've got a busy week ahead of me, but will take a look when I get the chance 🙇

[zachdaniel]

So @sevenseacat has made two good points about the current solution here:

1. we need to rollback these migrations in the dev & test databases before deleting them. I think (will need some experimentation) that we should do that with System.cmd("mix", ["ecto.rollback", "--to", "<the earliest migration id>]), and then again with MIX_ENV=test in the environment. We will also need to roll it back for any tenants if there are tenant migrations. The logic for that is a bit hairier but basically consists of, for each env, listing all tenants and and running the same command with --prefix <the tenant>.

2. we should add a variation of this task that works using git history. What it would do is mark all uncommitted migration files as dev migrations, and then go through this same path. Something like mix ash.codegen --squash-uncommitted

I think #1 will need to be a part of this PR because otherwise we'll be putting users dbs in a bad state (they can't roll back the migrations because they've been deleted).

#2 can be done in a followup.

[ken-kost]

alright, thanks for direction. working on #1, will remove last commit which is refactor, that's for seperate PR. 👍

[zachdaniel]

Do the tests cover the rollback behavior?

[ken-kost]

Do the tests cover the rollback behavior?

right, I'll expand it calling migrate. 👍

[ken-kost]

@zachdaniel
Okay so I'm having some trouble doing the migration in test, my idea was to assert it was made successfully both times after generating migrations, dev and regular.
What happens is that twice the same extension migration is made (for sandbox repo and for test repo) which breaks the migration. For instance if I do
Mix.Tasks.AshPostgres.Migrate.run(["--migrations-path", "test_migration_path"])
I would get

** (Ecto.MigrationError) migrations can't be executed, migration name migrate_resources_extensions_1 is duplicated

The duplication happens because of the two repos that create_extension_migrations runs on, making two same named extension migrations.
Also haven't found any test running migrate or rollback.? 🤔
Do you have an idea how should I solve this? I'm thinking somehow getting rid of the sandbox repo maybe, I'm not sure. 🐛

[zachdaniel]

Hmm...I think we should be making the sandbox and non sandbox repo use a different path for their migrations then?

[ken-kost]

Hmm...I think we should be making the sandbox and non sandbox repo use a different path for their migrations then?

Okay so it gets tricky. Because migration path for any migration is determined:

  defp migration_path(opts, repo, tenant? \\ false) do
    # Copied from ecto's mix task, thanks Ecto ❤️
    config = repo.config()

    if tenant? do
      if path = opts.tenant_migration_path || config[:tenant_migrations_path] do
        path
      else
        priv =
          AshPostgres.Mix.Helpers.source_repo_priv(repo)

        Path.join(priv, "tenant_migrations")
      end
    else
      if path = opts.migration_path || config[:tenant_migrations_path] do
        path
      else
        priv =
          AshPostgres.Mix.Helpers.source_repo_priv(repo)

        Path.join(priv, "migrations")
      end
    end
  end

So it's either configured through opts or is AshPostgres.Mix.Helpers.source_repo_priv(repo). So if migration_path is set then both repo's hit that path. If not then it hits existing migrations, but we are creating here temporary migrations in a temporary folder right, also, using existing macros like defposts trigger in migration a CREATE TABLE posts which already exists in existing migrations (I did circumvent this by adding a macro that creates a new table but I still had issues) .

This simple assertion turned out way harder to prove than anticipated. 😓

[ken-kost]

I'll work on it more, try to find a path. 🐛

[zachdaniel]

This should use:

Mix.Task.reenable("ash_postgres.rollback")
Mix.Task.run("ash_postgres.rollback", ["--repo", inspect(repo)])

(using your new --repo option). This also avoids needing to do anything special with MIX_ENV etc.

[ken-kost]

After doing this, if I leave

    if tenant? do
      Enum.each(repo.all_tenants(), fn tenant ->
        opts = ["--repo", inspect(repo), "--to", version, "--prefix", tenant]
        Mix.Task.run("ash_postgres.rollback", opts)
      end)
    end

then it fails. I get an error

     ** (Ash.Error.Unknown) 
     Bread Crumbs:
       > Error returned from: AshPostgres.MultitenancyTest.Org.read

which comes from repo.all_tenants()
and there is also a log

     The following output was logged:
     11:30:35.118 [info] Migrations already down
     11:30:35.123 [info] Migrations already down

So I assume that also being done.? 🤔 I removed it then.

[zachdaniel]

Added some feedback. We can get this over the line and I'll merge and figure out what to do about testing the rollback 😄

[ken-kost]

Now I'm thinking maybe the -r flag could also help with the testing. 🤔 I'll check it out end if so, what a fortunate coincidence 🍀

[ken-kost]

Okay so

      Mix.Tasks.AshPostgres.Migrate.run([
        "--migrations-path",
        "test_migration_path",
        "--repo",
        "AshPostgres.TestRepo"
      ])

worked, but only once. 😓 i.e. it works after test.reset and does not work the second time and also does not work when I run it again after the real generate migration.

And it worked if I commented out create_extension_migrations(repos, opts) and in test made async true use AshPostgres.RepoCase, async: true FYI

[zachdaniel]

I think you need to re-migrate as part of test teardown.