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

Add prefix option to check repo status plug #181

Conversation

stevehodgkiss
Copy link
Contributor

When developing an app that uses schema-based multi-tenancy, it would be useful to be able to configure another CheckRepoStatus plug for a named "dev" tenant:

    plug Phoenix.Ecto.CheckRepoStatus, otp_app: :myapp

    plug Phoenix.Ecto.CheckRepoStatus,
      otp_app: :myapp,
      migration_paths: &Myapp.Release.tenant_migrations_path/1,
      prefix: "tenant_acme"

migration_opts has been add to PendingMigrationError, so the configured prefix and migration_lock options are used when the "Run migrations for repo" action is triggered.

When using schema based multi-tenancy, prefix can be used to check pending migrations for a "dev" tenant
@@ -10,13 +10,14 @@ defmodule Phoenix.Ecto.CheckRepoStatus do
* `:otp_app` - name of the application which the repos are fetched from
* `:migration_paths` - a function that accepts a repo and returns a migration directory, or a list of migration directories, that is used to check for pending migrations
* `:migration_lock` - the locking strategy used by the Ecto Adapter when checking for pending migrations. Set to `false` to disable migration locks.
* `:prefix` - the prefix used to check for pending migrations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of prefix, what if we allow migration_paths to be {path, opts} pairs? This way you can specify opts per path?

Copy link
Contributor Author

@stevehodgkiss stevehodgkiss Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be more flexible & wouldn't require multiple plug configurations.

defmodule MyMod do
  def migrations_paths(repo) do
    default_migrations_path = Migrator.migrations_path(repo)

    tenant_migrations_path =
      Path.join(default_migrations_path, "../tenant_migrations") |> Path.expand()

    [default_migrations_path, {tenant_migrations_path, prefix: "tenant_acme"}]
  end
end

# endpoint

    plug Phoenix.Ecto.CheckRepoStatus,
      otp_app: :myapp,
      migration_paths: &MyMod.migrations_paths/1

Could also fetch the tenant schema names from the db rather than use a fixed value. Will update tomorrow 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR to allow tuples in migration_paths. There's a slight change in behaviour in 5648d73, where given fn -> ["a", "b"] end it'll run the migrator once for a and again for b.

I'm not sure how well the option name migration_paths fits with this new functionality. Alternative names could be migrate_runs or a migrate: fn -> ["a", {"b", prefix: "tenant"}]. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, I am sorry. I think the change in behaviour is not acceptable because it does change how the migrator sees and runs migrations. I think your original version is the way to go indeed. Can you please revert? And thank you for exploring this path. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted 6c36d02 which means the migrator will run once for the ["a", "b"] case (although the code to maintain this behaviour is a bit complicated). Oh, did you mean revert back to the original version with the prefix option (i.e. to include only the first 2 commits in this PR)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback to the original version. It would require you to list the plug twice but I think that's alright?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted to the original version. Yeah, listing the plug twice is fine for this use case (a known dev tenant prefix).

@stevehodgkiss stevehodgkiss force-pushed the add-prefix-option-to-check-repo-status-plug branch from 27b43be to 6c36d02 Compare October 25, 2024 06:05
@josevalim josevalim merged commit fe00d3d into phoenixframework:main Oct 25, 2024
4 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants