Mysql compatibility by maxmarcon · Pull Request #53 · ateliware/triplex

maxmarcon · 2018-02-06T21:04:10Z

Here's the PR for issue #52

Looking forward to your comments.

…ted accordingly

coveralls · 2018-02-06T21:10:11Z

Coverage decreased (-11.06%) to 88.945% when pulling 2e90ee4 on maxmarcon:mysql-compatibility-upstream into 65b84a8 on ateliware:master.

kelvinst · 2018-02-08T12:18:21Z

mix.exs

      {:ecto, "~> 2.1"},
      {:postgrex, ">= 0.11.0"},
-
+      {:mariaex, "~> 0.8.2"},


So, let's make this one and :postgrex dependencies optional, since the user will probably choose between one and another.

I kept only mariaex optional, because otherwise your test suite fails. It's weird, I would have expected the "optional" requirement to have an effect only if the project is included by another one: https://hexdocs.pm/mix/Mix.Tasks.Deps.html but I must be missing something.

kelvinst · 2018-02-08T12:23:37Z

lib/triplex.ex

+          case SQL.query(repo, "DELETE FROM #{Triplex.config().tenant_table} WHERE NAME = ?", [tenant]) do
+            {:error, msg} -> {:error, msg}
+            _ -> {:ok, tenant}
+          end


We could refactor this nested cases to use a with

kelvinst · 2018-02-08T12:25:30Z

lib/triplex.ex

      {:error, reserved_message(new_tenant)}
-    else
+    repo.__adapter__ == Ecto.Adapters.MySQL ->
+      {:error, "you cannot rename tenants in a MySQL database. You will have to drop the tenant and recreate it with a different name"}


Well, to drop the tenant and create another is not a good option, since the user will lose all his tenant data. Let's remove this part and stay only with: "you cannot rename tenants in a MySQL database"

Good point, changed.

kelvinst · 2018-02-08T12:26:29Z

lib/triplex.ex

+    sql = case repo.__adapter__ do
+      Ecto.Adapters.MySQL ->
+        "SELECT name FROM #{config().tenant_table}"
+      _ ->


Let's "let it fail" and put Ecto.Adapters.Postgres on this case clause ;)

kelvinst · 2018-02-08T12:26:54Z

lib/triplex.ex

+      sql = case repo.__adapter__ do
+        Ecto.Adapters.MySQL ->
+          "SELECT COUNT(*) FROM #{config().tenant_table} WHERE name = ?"
+        _ ->


Let's "let it fail" and put Ecto.Adapters.Postgres on this case clause ;)

maxmarcon · 2018-02-11T23:05:35Z

@kelvinst thanks for your comments. I addressed them the best I could.

I also did 2 additional things:

I adapted the tests to work under MySQL. There's some ugliness caused by DDL operations in MySQL automatically issuing a commit of the transaction. Avoiding this required changing the mode of the sandbox. I documented it in triplex_test.exs.
I modified the triplex.migrations task to list the state of the migrations for each tenant. So far, it wasn't really working because it was only using Ecto.Migrator.migrations, which gives you the list of migrations with their status but is not "tenant aware" (you can't pass it a prefix). Therefore, it was always showing the migrations as being down. I changed it to combine that information with the result of Ecto.Migrator.migrated_versions, which accepts a prefix and you can use to obtain the real status of the migrations in a tenant. Suggestion: it would be cool to be able to run/rollback migrations for only a specific tenant, too.

…t run

kelvinst · 2018-02-21T20:32:32Z

Hey @maxmarcon cool! Sorry for the delay, new job. 😬

I'll review it soon, I promise. 😄

kelvinst · 2018-03-09T13:12:34Z

lib/mix/tasks/triplex.init.tenant.ex

@@ -0,0 +1,58 @@
+defmodule Mix.Tasks.Triplex.Init.Tenant do


What about renaming this task to triplex.mysql.install?

…figuration in README

Max Marcon added 4 commits January 31, 2018 14:33

basic mysql support for Triplex.create_schema and Triplex.drop

2373b35

fixed Triplex.all to work with MySQL

ab064d3

added mix task to initialize tenant table

af85747

triplex can now create and drop tenants with mysql, tenant table upda…

20821b6

…ted accordingly

Max Marcon added 2 commits February 7, 2018 11:06

better Mariaex error handling

894b2df

added mariaex to mix depnendencies

0607c7c

kelvinst suggested changes Feb 8, 2018

View reviewed changes

Max Marcon and others added 6 commits February 9, 2018 16:14

removed unused alias

5bc7aa9

slowly taming the tests

11f4ba2

triplex.migrations correctly shows the migration status for each tenant

b95e4e1

for MySQL, add tenant to tenants table in create_schema

ad61d96

all tests now work with MySQL

43d6c6e

addressed comments in code review

bedb1bb

keeping postgres dependency non-optional otherwise test pipeline won'…

923eb6f

…t run

kelvinst reviewed Mar 9, 2018

View reviewed changes

kelvinst approved these changes Mar 9, 2018

View reviewed changes

renamed triplex.init.tenant to triplex.mysql.install, added MySQL con…

2e90ee4

…figuration in README

kelvinst approved these changes Jun 19, 2018

View reviewed changes

mgiacomini approved these changes Jun 19, 2018

View reviewed changes

mgiacomini merged commit bc80229 into ateliware:master Jun 19, 2018

mgiacomini mentioned this pull request Jun 19, 2018

Making Triplex MySQL compatible #52

Closed

Conversation

maxmarcon commented Feb 6, 2018

Uh oh!

coveralls commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxmarcon commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kelvinst commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 6, 2018 •

edited

Loading

maxmarcon commented Feb 11, 2018 •

edited

Loading