get_migrations_function of Phoenix.Ecto.CheckRepoStatus is not used for running migrations

[dvic]

I have something like this

    plug Phoenix.Ecto.CheckRepoStatus,
      otp_app: :playground_site,
      get_migrations_function: &PlaygroundSite.Migrations.migrations/1

where

defmodule PlaygroundSite.Migrations do
  def migrations(repo) do
    Ecto.Migrator.migrations(repo, ["priv/repo/migrations", "../my_project/priv/repo/migrations"])
  end
end

The result is that only the migrations in priv/repo/migrations are run because the get_migrations_function option is only used to check whether there are migrations to be run. The actual running of the migrations is done using Ecto.Migrator.run(repo, :up, all: true), which only looks at the migrations in the priv folder.

Earlier today, I posted a suggestion on the ecto mailing list which would solve this issue: https://groups.google.com/g/elixir-ecto/c/y41IAhOKZZk/m/i4YpOdxcHgAJ

I'm happy to work on a solution for this if there's interest.

[josevalim]

Ecto.Migrator.run already allows a list of paths to be given as second argument. Or even a list of migrations. You can use any of said options. A PR is welcome!

[dvic]

I just looked into this but I'm running into problems with one thing: the get_migrations_function function has to return migrations, so I call Ecto.Migrator.migrations(repo, directories), which returns [{:up | :down, id :: integer(), name :: String.t}].

However, Ecto.Migrator.run(repo, migration_source, direction) expects for migration_source a String.t | [String.t] | [{integer, module}], which I don't directly get from the get_migrations_function function and I don't see any easy (public) function to make this conversion.

What is the best way forward here? In the description I reference a post I made on the mailing list where I suggested to support multiple migration paths for a repo using configuration. The following does the trick in a backwards compatible way: https://github.com/elixir-ecto/ecto_sql/commit/23908d2963e406e00b66ed63bfffd3f0c7b61f38. Then we could deprecate this get_migrations_function because then you would have the multiple migration paths configured for the repo itself, and all operations use these multiple migration paths.

[josevalim]

So we should be able to convert the migrations to the format that Migration.run needs, right?

[dvic]

Yes but I'm not sure that is possible? Because with only [{:up | :down, id :: integer(), name :: String.t}] you lose the information from which directories these migrations came from, right? So how can you find the corresponding module name?

(we either need to produce directories :: [String.t] or version_plus_modules :: [{integer, module}])

[josevalim]

Ah, the module is missing. I think we should deprecate the get_migrations_function into something that allows multiple paths and support multiple paths in the Plug?

the trick with supporting it officially on Ecto is that you almost always want to treat them as separate instead of a single one so you run them at distinct moments.

[dvic]

Ah, the module is missing. I think we should deprecate the get_migrations_function into something that allows multiple paths and support multiple paths in the Plug?

Yes, this would be an option and would only require a change in phoenix_ecto. The return type of this function would have to be String.t | [String.t] so that I can call Ecto.Migrator.migrations(repo, directories) inside the CheckRepoStatus plug and pass it on when raising the PendingMigrationError.

Should I emit a deprecation warning when this function is passed and support a new option called migration_souce?

the trick with supporting it officially on Ecto is that you almost always want to treat them as separate instead of a single one so you run them at distinct moments.

I'm not sure I follow, what are you referring to in this sentence with "supporting it officially on Ecto" and "treat them as separate" :) ? (as I understand the above approach only requires a change in phoenix_ecto)

[josevalim]

Should I emit a deprecation warning when this function is passed and support a new option called migration_souce?

yes, let’s call The new option migration_paths.

I'm not sure I follow, what are you referring to in this sentence with "supporting it officially on Ecto" and "treat them as separate" :) ?

I meant about your Ecto mailing list proposal. When we have several migrations paths, we often want to run those migrations at distinct moments. So squashing them together in a single option does not necessarily help!

[dvic]

yes, let’s call The new option migration_paths.

Alrighty! Will try to get a PR out today.

I meant about your Ecto mailing list proposal. When we have several migrations paths, we often want to run those migrations at distinct moments. So squashing them together in a single option does not necessarily help!

I see, haven't thought about that use-case, in our case we do actually want to run them together :) In our case we have reusable applications that carry their own migrations (so we can properly test in isolation) and we want to pick and choose the applications in a parent "tenant" application (each tenant has different features / applications). So then it would be nice if the Repo of each tenant looks at all the folders of each sub-application by default. Like I said, this already works on the command-line (so creating the appropriate aliases in mix.exs should work) but it seems to me that this should also be supported on the config level of things.

[josevalim]

I see... in this case a migration_paths configuration that takes higher precedence to migration_path in Ecto would also be welcome as an additional PR. :)

[dvic]

I see... in this case a migration_paths configuration that takes higher precedence to migration_path in Ecto would also be welcome as an additional PR. :)

Cool, I'll cook up a PR for that as well (in ecto_sql). A question about that: currently, Ecto.Migrator has a public function

  @spec migrations_path(Ecto.Repo.t, String.t) :: String.t
  def migrations_path(repo, directory \\ "migrations") do
    config = repo.config()
    priv = config[:priv] || "priv/#{repo |> Module.split |> List.last |> Macro.underscore}"
    app = Keyword.fetch!(config, :otp_app)
    Application.app_dir(app, Path.join(priv, directory))
  end

which is used in places like

  @spec run(Ecto.Repo.t, atom, Keyword.t) :: [integer]
  def run(repo, direction, opts) do
    run(repo, [migrations_path(repo)], direction, opts)
  end

I feel that run(repo, [migrations_path(repo)], direction, opts) should be replaced with something like run(repo, migration_paths(repo), direction, opts) (note the difference between migrationS_path and migratioN_pathS)

But what do I do with this migrations_path function? Change only migrations_path/1 and make the return type return mulitple paths in case it's configured in the config? And keep migrations_path/2 how it is now? (i.e., generate path for a given subfolder inside the repo's priv folder). Or deprecate migrations_path/1 and migrations_path/2 altogether and introduce a new migration_paths/1 that reads the config and has fallback to what is now migrations_path(repo, "migrations")?

Once we decide on this, I can call this new migration_paths instead of the hardcoded (kind of code-duplicated) fallback [Path.join(source_repo_priv(repo), "migrations")] inside Mix.EctoSQL's ensure_migrations_paths function :

  @spec ensure_migrations_paths(Ecto.Repo.t, Keyword.t) :: [String.t]
  def ensure_migrations_paths(repo, opts) do
    paths = Keyword.get_values(opts, :migrations_path)
    paths = if paths == [], do: [Path.join(source_repo_priv(repo), "migrations")], else: paths

    if not Mix.Project.umbrella?() do
      for path <- paths, not File.dir?(path) do
        raise_missing_migrations(Path.relative_to_cwd(path), repo)
      end
    end

    paths
  end

This function is called by the various mix migration tasks to get the migration folders. I assume that we want to have the command-line arguments override the config / repo assumed defaults, like it is now the case.

If we can change in the above [Path.join(source_repo_priv(repo), "migrations")] to Ecto.Migrator.migration_paths(repo) then everything is nice and consistent :)

[josevalim]

Sorry, that's getting too complex which is why I decided to postpone it for now. Let's go only with the PhoenixEcto change meanwhile? :)

Note to future self: we should go with migrations_paths for the naming for consistency.

[dvic]

Sorry, that's getting too complex which is why I decided to postpone it for now. Let's go only with the PhoenixEcto change meanwhile? :)

Oh shoot, I shouldn't have typed that much 🤓 hah I'm kidding, I understand, let's go just for the phoenix_ecto change then.

[josevalim]

Closing in favor of PR!