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

fix: create roles before migration #222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pdgonzalez872
Copy link

@pdgonzalez872 pdgonzalez872 commented Dec 13, 2023

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! ❤️

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. ❤️

@pdgonzalez872 pdgonzalez872 changed the title fix: create roles in test db as well fix: create roles before migration Dec 13, 2023
@pdgonzalez872
Copy link
Author

@abc3 can we merge this one or do you have any additional concerns?

@abc3
Copy link
Member

abc3 commented Jun 12, 2024

Sorry, I haven't had a chance to check yet. I will take a look soon

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