Ecto 3 support #61

abarr · 2018-11-06T03:11:04Z

Hi,

I am working on a project that we wanted to upgrade to Ecto 3.0.1. I have made changes to Triplex so we can continue development.

I could not find any guidance for submitting Pull Requests so I have followed the general conventions )However, this is my first OSS submission).

I have not updated the README as I am not sure how you want to reference eth changes.

I also could not get the tests to run outside of a project but they have passed when I have added it as a dependency to our project.

So far all use cases we used are working as expected. If I find any issues I will update.

I hope this helps.

Andrew

mgiacomini · 2018-11-06T12:09:56Z

@abarr could you fix the tests? i will checkout this and try to run locally :)

abarr · 2018-11-06T22:41:55Z

@mgiacomini I am new to contributing to open source so please be patient ...

I had a look at the Travis log and it says that the tests are being run with a config of elixir 1.4.2. I had to upgrade Triplex to use elixir 1.6 so it can use Ecto 3. Are you able to change the Travis config?

abarr · 2018-11-06T22:42:31Z

I will also have a go at getting the tests running locally

kapilpipaliya · 2018-11-09T16:46:04Z

Triplex.create(tenant) can't run migrations, when inside Transaction. it says (invalid_schema_name) schema "foo" does not exist
but It created schema first like
CREATE SCHEMA "foo" []

If I run Triplex.create(tenant) outside transaction it work successfully.

you can test running: Repo.transaction(fn -> Triplex.create("foo") end)

kapilpipaliya · 2018-11-12T11:19:24Z

workaround is do not use Triplex.create("foo") in Transaction.

mgiacomini · 2018-11-12T12:31:20Z

workaround is do not use Triplex.create("foo") in Transaction.

Yeah. I think the postgres doesn't support creating schemas/databases inside transactions, because we need to tell where it will run the operations, so if the schema/database doesn't exists it's not able to decide it.

kapilpipaliya · 2018-11-14T04:20:07Z

@mgiacomini but Repo.transaction(fn -> Triplex.create("foo") end) work with Ecto 2, so its already supported.

mgiacomini · 2018-11-14T15:54:57Z

@kapilpipaliya humm, so I need a close look into this.

dhonig · 2019-01-31T21:25:29Z

Any chance of this getting merged? I've just started a project using Ecto 3 and @abarr's work and would like to see Ecto 3 support added.

churcho · 2019-02-20T11:42:28Z

Any chance on getting this track resolved? I am getting

(DBConnection.ConnectionError) connection not available and request was dropped from queue
 after 970ms. You can configure how long requests wait in the queue using 
:queue_target and :queue_interval`

when I run Triplex tests on @abarr's branch as well.

kelvinst · 2019-02-22T13:19:29Z

lib/mix/tasks/triplex.mysql.install.ex

@@ -27,7 +27,7 @@ defmodule Mix.Tasks.Triplex.Mysql.Install do
        Mix.raise "the tenant table only makes sense for MySQL repositories"
      end

-      path = Path.relative_to(Mix.Ecto.migrations_path(repo),
+      path = Path.relative_to(Mix.EctoSQL.ensure_migrations_path(repo),


Well, I see there is no more a migrations_path function on the new ecto_sql lib, I think it's cool anyway.

This is private API and shouldn't be used by external tools. Use Ecto.Migrator.migrations_path/1 instead.

kelvinst · 2019-02-22T13:22:04Z

lib/triplex.ex

+    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"))


Not sure of the utility of this refactor here. Does the repo config changed to a map? I mean, I get that one could be confuse with the change of main value on the pipelines, is that the reason you did it?

PS.: Just out of curiosity, I think it got more readable anyway, so let's leave it your way.

You can use https://hexdocs.pm/ecto_sql/3.0.5/Ecto.Migrator.html#migrations_path/1 and just replace last element with tenant_migrations.

coveralls · 2019-02-22T15:32:55Z

Coverage increased (+10.5%) to 99.507% when pulling e62ceb2 on abarr:ecto_3_support into f49c43f on ateliware:master.

kelvinst · 2019-02-22T19:39:19Z

I got what's the problem with transaction: in Ecto 3 they changed the execution of migrations to run them on another process, and by default, when you run a transaction, it links the connection to the current process, and if you start another process inside of it, that process will not use the same connection, so everything done inside the other process will be in another transaction and therefore not get anything done by the original one.

I'm checking if there is a way to solve that.

kelvinst · 2019-02-22T21:55:52Z

@abarr I merged master to your branch and that broke the tests, so I did some changes to make the tests pass. You can review them on my commits for your branch.

@kapilpipaliya About the transaction problem, I explained on the previous comment. Created the issue
elixir-ecto/ecto_sql#84 for ecto people, since the only solution I see would have to add an option for Ecto.Migrator, as for how it is now, there is no way to run Triplex.create inside a transaction, so we will probably not release this any time soon, unfortunately.

kelvinst · 2019-02-22T22:18:24Z

So, José answered the issue and he would not like to change what we need on ecto, because it would add too much complexity there. So I had another idea, which will maybe solve #47 too! Instead of running migrations when creating tenants, use a structure.sql dump file for it. It's a little bit more complex, but I guess it will pay off because it will make it a lot faster too.

kelvinst · 2019-02-23T09:57:25Z

lib/mix/tasks/triplex.migrate.ex

+
+      for prefix <- Triplex.all(repo) do
+        new_args = delete_unwanted_args(args, [])
+        Mix.Task.run("ecto.migrate", ["--repo", inspect(repo), "--prefix", prefix | new_args])


I totally forgot this don't work. :(

This way it does not run the migrations inside the tenant_migrations folder, and yes on migrations folder.

Maybe we should change that too, for the new version of triplex. Instead of having a tenant_migrations folder, we could put all migrations inside migrations and if someone would not like to filter out migrations on public or tenant schemas one could do somthing like:

def change do if Triplex.reserved_tenant?(prefix()) do # run your public schema migration else # run your tenant schema migration end end

@mgiacomini what do you think? We would then totally break the current working version. The dump change would break it anyways, but the solution for that would be just to run a triplex.dump, this one will take a lot more trouble for users to port to the new version, since they would have to add an if inside all of their tenant migrations.

The other solution would be rolling back to the old way of running migrations, but that's a trouble for us then, since any change on the migration stuff on ecto's side, we would have to change ours too.

So, looks like the change to use only one migrations folder is not possible at all, and we will have to make it work with tenant_migrations. I'll delete my commits til now since they are all in favor of the idea of using ecto.migrate --prefix, and that will not be possible at all.

@kelvinst was the migration fixed?
I can't run mix triplex.migrate

How does one pass a default tenant name to migrations to ensure migrations are run for a potentially pre-existing tenant(s)?

@kelvinst was the migration fixed?
I can't run mix triplex.migrate

Where are you running them? Is it your project? Are you pointing the dep to this PR branch?

How does one pass a default tenant name to migrations to ensure migrations are run for a potentially pre-existing tenant(s)?

That's pretty much what mix triplex.migrate does. It runs all the migrations for all the pre-existing tenants, not for new ones. Migration for the new ones a ran when you call Triplex.create

hauleth · 2019-02-25T17:58:14Z

lib/triplex.ex

+    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"))


You can use https://hexdocs.pm/ecto_sql/3.0.5/Ecto.Migrator.html#migrations_path/1 and just replace last element with tenant_migrations.

mix.exs

config/test.exs

hauleth · 2019-02-25T18:01:07Z

lib/mix/tasks/triplex.mysql.install.ex

@@ -27,7 +27,7 @@ defmodule Mix.Tasks.Triplex.Mysql.Install do
        Mix.raise "the tenant table only makes sense for MySQL repositories"
      end

-      path = Path.relative_to(Mix.Ecto.migrations_path(repo),
+      path = Path.relative_to(Mix.EctoSQL.ensure_migrations_path(repo),


This is private API and shouldn't be used by external tools. Use Ecto.Migrator.migrations_path/1 instead.

lib/mix/triplex.ex

lib/mix/tasks/triplex.rollback.ex

kelvinst · 2019-02-26T11:57:08Z

@hauleth thanks for the review. I knew some of the functions are private API there. I switched the ones I could to other versions of it, but in some cases that's not possible because the public API is not the same, as for source_priv_dir for example, it returns the source code path, and Ecto.Migrator.migration_path returns it from inside the _build dir. I didn't dig into it if we could really change that, but as it is not the intention of this PR, I have created another issue for it: #62

hauleth · 2019-02-26T12:50:11Z

@kelvinst you can check Mix.EctoSQL source and copy respective functions as these should use only public API of Ecto.

kelvinst · 2019-02-26T13:04:05Z

@kapilpipaliya and @abarr I had another idea on the transaction problem: we will make it clear we don't support that on the documentation, but will also give a way for users to guarantee the schema will get dropped if some of their stuff fails. Check 033d6d4 for details.

kelvinst · 2019-02-26T13:07:13Z

@kelvinst you can check Mix.EctoSQL source and copy respective functions as these should use only public API of Ecto.

Yep, I know, will do that on another PR though, since this is an old problem and there is already too much stuff on this PR.

kelvinst · 2019-02-26T13:08:42Z

@churcho can you test this PR again? Maybe it got solved by my fixes for it.

Also, changing most imports to aliases

kelvinst · 2019-02-27T10:54:56Z

@hauleth at the end of the day I also did what you suggested, copied the code from Mix.EctoSQL.

This PR can be considered done now, I'm only doing some final touches for contributing and changelog files, and then I'll merge and release a new version!

Andrew Barr added 2 commits November 6, 2018 12:16

Updated Triplex to use Ecto 3.0.1

ae640ca

Removed option flag from MariaEx

27486cc

Andrew Barr added 4 commits November 7, 2018 16:35

Updated references from Ecto.MIgrator to EctoSQL.MIgrator

64714dc

Rolled back changes from Ecto.MIgrator to EctoSQL.MIgrator

3e0d51d

Updated migrations_path to reflect changes in ecto_sql

737986c

upped version

39e3fe4

rwngallego mentioned this pull request Jan 21, 2019

Ecto 3 Support #59

Closed

kelvinst previously approved these changes Feb 22, 2019

View reviewed changes

Merge branch 'master' into ecto_3_support

8342ac2

kelvinst dismissed their stale review via 8342ac2 February 22, 2019 13:25

kelvinst mentioned this pull request Feb 22, 2019

Problem with the async migrations elixir-ecto/ecto_sql#84

Closed

kelvinst reviewed Feb 23, 2019

View reviewed changes

hauleth suggested changes Feb 25, 2019

View reviewed changes

kelvinst added 3 commits February 25, 2019 17:34

Fix migration after ecto 3 migration

50e262f

Fixing some code climate issues

17fd149

Some refactors on migrator functions

c09e1f5

kelvinst added 4 commits February 25, 2019 19:37

Using the less we can from private API's

1d2f228

Running MySQL tests everytime (with no code change needed)

555647c

Running formatter

49766e9

Dropping tenant when it fails on the func execution

4dfcff5

Fixing code climate issues

a1466f4

Documenting the transactions problem

033d6d4

Not using private APIs anymore

2d96a74

Also, changing most imports to aliases

Contribution guide, changelog, some refactors and improve coverage

e62ceb2

mgiacomini approved these changes Feb 28, 2019

View reviewed changes

kelvinst merged commit 37d2e42 into ateliware:master Mar 3, 2019

kelvinst mentioned this pull request Mar 4, 2019

Do not use Ecto's private APIs #62

Closed

0urobor0s mentioned this pull request Mar 25, 2019

Creating tenant in tests #67

Closed

PaulOstazeski mentioned this pull request Jul 11, 2023

Remove error from mix task triplex.migrations #91

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ecto 3 support #61

Ecto 3 support #61

abarr commented Nov 6, 2018

mgiacomini commented Nov 6, 2018

abarr commented Nov 6, 2018

abarr commented Nov 6, 2018

kapilpipaliya commented Nov 9, 2018 •

edited

Loading

kapilpipaliya commented Nov 12, 2018

mgiacomini commented Nov 12, 2018 •

edited

Loading

kapilpipaliya commented Nov 14, 2018

mgiacomini commented Nov 14, 2018

dhonig commented Jan 31, 2019

churcho commented Feb 20, 2019 •

edited

Loading

kelvinst Feb 22, 2019

hauleth Feb 25, 2019

kelvinst Feb 22, 2019

hauleth Feb 25, 2019

coveralls commented Feb 22, 2019 •

edited

Loading

kelvinst commented Feb 22, 2019

kelvinst commented Feb 22, 2019

kelvinst commented Feb 22, 2019

kelvinst Feb 23, 2019

kelvinst Feb 23, 2019

kelvinst Feb 23, 2019

kelvinst Feb 25, 2019

churcho Mar 1, 2019 •

edited

Loading

kelvinst Mar 2, 2019

hauleth Feb 25, 2019

hauleth Feb 25, 2019

kelvinst commented Feb 26, 2019

hauleth commented Feb 26, 2019

kelvinst commented Feb 26, 2019

kelvinst commented Feb 26, 2019

kelvinst commented Feb 26, 2019

kelvinst commented Feb 27, 2019

Ecto 3 support #61

Ecto 3 support #61

Conversation

abarr commented Nov 6, 2018

mgiacomini commented Nov 6, 2018

abarr commented Nov 6, 2018

abarr commented Nov 6, 2018

kapilpipaliya commented Nov 9, 2018 • edited Loading

kapilpipaliya commented Nov 12, 2018

mgiacomini commented Nov 12, 2018 • edited Loading

kapilpipaliya commented Nov 14, 2018

mgiacomini commented Nov 14, 2018

dhonig commented Jan 31, 2019

churcho commented Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 22, 2019 • edited Loading

kelvinst commented Feb 22, 2019

kelvinst commented Feb 22, 2019

kelvinst commented Feb 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

churcho Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelvinst commented Feb 26, 2019

hauleth commented Feb 26, 2019

kelvinst commented Feb 26, 2019

kelvinst commented Feb 26, 2019

kelvinst commented Feb 26, 2019

kelvinst commented Feb 27, 2019

kapilpipaliya commented Nov 9, 2018 •

edited

Loading

mgiacomini commented Nov 12, 2018 •

edited

Loading

churcho commented Feb 20, 2019 •

edited

Loading

coveralls commented Feb 22, 2019 •

edited

Loading

churcho Mar 1, 2019 •

edited

Loading