supavisor
supavisor copied to clipboard
fix: create roles before migration
Hi! Thanks for working on this!
I wanted to run tests (mix test) in the project and hit a couple of snags in a fairly fresh vanilla Ubuntu install (I can give details about them if you'd like, but it is not related to this issue at hand. There are a couple of dependencies that I didn't have, but were straightforward to add (cargo, cmake, llvm and friends).
Once everything is set up and we are able to run things, there is a check in the priv/repo/seeds_after_migration.exs that will never succeed (making sure the roles are created). It's because we don't create them in tests, we only do it in dev (https://github.com/supabase/supavisor/blob/main/dev/postgres/00-setup.sql#L1-L3). At least, this is what I think at the moment (I may be wrong).
What kind of change does this PR introduce?
Small test setup change, allows to run a mix test from the project root.
What is the current behavior?
I got the following error when running locally:
10:40:42.512 [info] == Running 20231004133121 Supavisor.Repo.Migrations.AddDefaultPoolStrategy.change/0 forward file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
10:40:42.512 [info] alter table _supavisor.tenants file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
10:40:42.513 [info] create check constraint default_pool_strategy_values on table _supavisor.tenants file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
10:40:42.514 [info] == Migrated 20231004133121 in 0.0s file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
** (MatchError) no match of right hand side value: {:error, :rollback}
priv/repo/seeds_after_migration.exs:66: (file)
(elixir 1.15.4) lib/code.ex:1435: Code.require_file/2
What is the new behavior?
The roles are created in the test db successfully.
Additional context
Debugging sesh:
Add this diff (where my pg lives):
diff --git a/config/test.exs b/config/test.exs
index 66e90b2..e8cbf5f 100644
--- a/config/test.exs
+++ b/config/test.exs
@@ -19,7 +19,7 @@ config :supavisor, Supavisor.Repo,
database: "supavisor_test#{System.get_env("MIX_TEST_PARTITION")}",
pool: Ecto.Adapters.SQL.Sandbox,
pool_size: 10,
- port: 6432
+ port: 5432
Try mix test:
10:40:42.512 [info] == Running 20231004133121 Supavisor.Repo.Migrations.AddDefaultPoolStrategy.change/0 forward file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
10:40:42.512 [info] alter table _supavisor.tenants file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
10:40:42.513 [info] create check constraint default_pool_strategy_values on table _supavisor.tenants file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
10:40:42.514 [info] == Migrated 20231004133121 in 0.0s file=lib/ecto/migration/runner.ex line=342 pid=<0.961.0>
** (MatchError) no match of right hand side value: {:error, :rollback}
priv/repo/seeds_after_migration.exs:66: (file)
(elixir 1.15.4) lib/code.ex:1435: Code.require_file/2
Add inspects with this diff:
diff --git a/priv/repo/seeds_after_migration.exs b/priv/repo/seeds_after_migration.exs
index 58b1ce4..2e8fef0 100644
--- a/priv/repo/seeds_after_migration.exs
+++ b/priv/repo/seeds_after_migration.exs
@@ -77,5 +77,7 @@ end)
"grant all on table public.test to postgres;",
"grant all on table public.test to authenticated;"
]
- |> Enum.each(&query(Repo, &1, []))
+ |> Enum.each(fn el ->
+ query(Repo, el, []) |> IO.inspect(label: "SHEEP")
+ end)
end)
We get:
# all previous are successes
SHEEP: {:ok,
%Postgrex.Result{
command: :create_table,
columns: nil,
rows: nil,
num_rows: 0,
connection_id: 1536665,
messages: []
}}
SHEEP: {:error,
%Postgrex.Error{
message: nil,
postgres: %{
code: :undefined_object,
line: "5184",
message: "role \"anon\" does not exist",
file: "acl.c",
unknown: "ERROR",
severity: "ERROR",
routine: "get_role_oid",
pg_code: "42704"
},
connection_id: 1536665,
query: nil
}}
# all below are errors, since this is a Repo.transaction/1 call
I am getting a different error at the moment, but seems to be that my current Erlang install (26.0.2) didn't bring in the ct_slave module. Seems like there was some discussion about it here: https://github.com/supabase/supavisor/pull/31/files#r1145024804
To keep this issue smaller, I replaced the ct_slave call with the peer one, expecting to get failed test. And that "works", the tests run.
Hope this helps! Again, thanks for your work! :heart:
edit: I realize that the commit message and some of the wording above may incorrectly categorize this issue as a "test" db issue, but in fact, it's any db that it runs in. I approached this problem from "Cool project, let me run the tests" starting point. Please feel free to change the commit wording, the code, anything you'd like and also, if you don't use this, no problem at all. :heart:
@abc3 can we merge this one or do you have any additional concerns?
Sorry, I haven't had a chance to check yet. I will take a look soon