Skip to content
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

Allow the migration_path and snapshot_path to be configured per repo. #426

Closed
jimsynz opened this issue Nov 20, 2024 · 4 comments
Closed
Labels
enhancement New feature or request needs review

Comments

@jimsynz
Copy link
Contributor

jimsynz commented Nov 20, 2024

An APS client with a number of repos in their app recently asked if it's possible to have ash_postgres honour Ecto's :priv config value. I think we should both honour that value, but also allow configuration of the migration path and snapshot path on a per-repo basis.

@zachdaniel
Copy link
Contributor

AFAIK this is configurable per repo. I will look into it when I'm back at a computer though.

@zachdaniel
Copy link
Contributor

@jimsynz is there a reproduction or a specific example of the issue/case where we weren't honoring this option? Looking at the code:

  def derive_migrations_path(repo) 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, "migrations"))

  ...
  def derive_tenant_migrations_path(repo) 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, "tenant_migrations"))
    ...

So it's clear we're at least attempting to honor the repo's priv config value. Will need more info on what specifically illustrated that we were not.

@zachdaniel
Copy link
Contributor

nvm, we don't use it for snapshots.

@zachdaniel
Copy link
Contributor

But we do allow the migrations_path and tenant_migrations_path, so from what I can tell its just missing corresponding behavior in the migration generator.

    opts[:migrations_path] || repo.config()[:migrations_path] || derive_migrations_path(repo)
  end

  def tenant_migrations_path(opts, repo) do
    opts[:migrations_path] || repo.config()[:tenant_migrations_path] ||
      derive_tenant_migrations_path(repo)
  end

pshoukry pushed a commit to pshoukry/ash_postgres that referenced this issue Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs review
Projects
None yet
Development

No branches or pull requests

2 participants