-
Notifications
You must be signed in to change notification settings - Fork 330
Add :migrations_paths repo configuration option
#695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Currently, while multiple `--migrations-path` CLI arguments can be passed to `mix ecto.migrate` and related tasks, there is no way to configure multiple migration paths at the repository level. The only configuration option is `:priv`, which allows customizing a single directory that defaults to `"priv/repo/migrations"`. This limitation causes issues in projects with multiple repositories that need different or multiple migration paths for each repo. Each time migrations are run, the paths must be explicitly specified via CLI flags, making it inconvenient and error-prone, especially when using `mix ecto.migrate` without arguments or in automated deployment scripts. This commit introduces a new `:migrations_paths` configuration option that accepts a list of migration directory paths. When configured, these paths are used by default for all migration-related operations unless overridden by CLI flags. Configuration example: ```elixir config :my_app, MyApp.Repo, migrations_paths: ["priv/repo/migrations", "priv/repo/tenant_migrations"] ``` Relative paths are resolved against the application root and absolute paths are supported as well.
lib/ecto/migrator.ex
Outdated
|
|
||
| case config[:migrations_paths] do | ||
| nil -> | ||
| [migrations_path(repo)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should actually move the logic of migrations_path here and reimplement migrations_path as hd(migrations_paths(...)), adding a note it is now discouraged. Otherwise you can have code using migrations_path and migrations_paths and working with two completely set of directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept migrations_path unchanged to ensure backwards compatibility. If we call hd(migrations_paths(...)) and the :migrations_paths is set with more than one paths then it will only return the first one.
Should we raise in such cases? e.g.
def migrations_path(repo, directory) do
if is_list(config[:migrations_paths]) && length(config[:migrations_paths]) > 1 do
raise "multiple migrations paths defined, use migrations_paths/1 instead"
end
hd(...)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raising is good! But please use pattern matching! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, in order to handle the optional directory argument expected by migrations_path/1 I used an opts in migrations_paths/1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I would prefer to not expose the directory option, since it doesn't work with the configuration, so perhaps we have to refactor this into a private function where we keep the directory hidden.
I believe the directory though was added for people who want to have additional migrations directories, but not plug them all by default. So this pull request has the unfortunate side-effect of making those cases harder. :( And now I am not sure how to move forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#245 introduced this.
If you ask me, since the mix tasks support passing multiple migrations paths, we should only have migrations_paths. We could keep migrations_path/1 as it is for backwards compatibility or soft deprecate is.
Currently if you call migrations_path with a different directory then you will need to pass it explicitly in the ecto.migrate --migrations-path in order to be applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's how some projects tackle it indeed. In production you run them at distinct moments, so you don't want the additional path as default. And then you have mix tasks/aliases so you don't specify all of them:
[
"ecto.migrate.data": "migrate --migration-path foo/bar",
"ecto.migrate.all": "migrate --migration-path foo/bar --migration-path foo/baz"
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or I could bring its implementation back and have something like:
def migrations_path(repo, directory \\ "migrations") do
config = repo.config()
if is_list(config[:migrations_paths]) do
IO.warn("some warning....")
end
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
def migrations_paths(repo) do
case config[:migrations_paths] do
nil -> [migrations_path(repo)]
...
end
end
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main problem is when you have multiple repos:
with the suggested change
config :my_app, ecto_repos: [Repo1, Repo2, Repo3]
config :my_app, Repo1, migrations_paths: ["foo", "bar", "../baz"]
config :my_app, Repo2, migrations_paths: ["foo2", "bar2"]
config :my_app, Repo3 # no migrations path the default priv/migrations is used
mix ecto.migrate # works automatically for all repos
# similarly the
mix ecto.migrate -r Repo1
currently
you need to create an for each repo alias and re-alias ecto.migrate or create another alias in order to work
[
"ecto.migrate.repo1": "migrate -r Repo1 --migration-path foo --migration-path bar ...",
"ecto.migrate.repo2": "migrate -r Repo2 --migration-path foo2 ...",
"ecto.migrate.repo3": "migrate -r Repo3"
"ecto.migrate.all": [
"ecto.migrate.repo1", "ecto.migrate.repo2", "ecto.migrate.repo3"
]
]
which works but is not convenient and prone to errors, e.g. when you want to add a new repo, or use the same repo in multiple apps
Currently, while multiple
--migrations-pathCLI arguments can be passed tomix ecto.migrateand related tasks, there is no way to configure multiple migration paths at the repository level. The only configuration option is:priv, which allows customizing a single directory that defaults to"priv/repo/migrations".This limitation causes issues in projects with multiple repositories that need different or multiple migration paths for each repo. Each time migrations are run, the paths must be explicitly specified via CLI flags, making it inconvenient and error-prone, especially when using
mix ecto.migratewithout arguments or in automated deployment scripts.This commit introduces a new
:migrations_pathsconfiguration option that accepts a list of migration directory paths. When configured, these paths are used by default for all migration-related operations unless overridden by CLI flags.Configuration example:
Relative paths are resolved against the application root and absolute paths are supported as well.