SSL Options on supervisor connection

[victorolinasc]

Because of this line I can't pass the valid ssl_opts keyword list to Rihanna, making it impossible to be used with ssl for myself.

IMHO the fix would simply be to add :ssl_opts here since you already take :ssl option. This would work for MY scenario, but it is important to notice that there are several other options that Postgrex accepts for starting a connection. So, the proper fix here might be to add all of them.

Without this I can't use Rihanna. Since we are not actually testing Rihanna with all possible connection setups, I can open a PR only adding the key if desired. Though it would be better if a maintainer could do it because, if possible, I'd need that on a PATCH or MINOR release.

Thanks for your work!

[lpil]

Thanks for the report! Let's get this fixed.

[victorolinasc]

Hi @lpil ! Me again. Sorry for all the trouble.

The SSL configuration is properly working now, but with taking all the options, I now have to exclude the pool configuration when I am running tests. Otherwise it tries to start with Ecto.Adapters.SQL.Sandbox which won't work on this line. It tries to create a connection with the pool being Ecto.Adapters.SQL.Sandbox though Sandbox does not implement a child_spec function. In the end it fails here.

Anyway, all I am currently doing is:

    Supervisor.start_link(
      [
        MyApp.Repo,
        {Rihanna.Supervisor, [postgrex: MyAppRepo.config() |> Keyword.drop([:pool])]}
      ],
      strategy: :one_for_one,
      name: MyApp.Supervisor
    )

In my humble opinion, the way things are now is the right approach, but people might get surprised for their tests failing because of this change. So, here I am trying to warn you people. Maybe the best approach would be to ONLY take extra :ssl_opts for this not to be a breaking change.

I'd advise to rollback this solution to the one only taking the :ssl_opts and retire version 1.3.3 from hex as it will break current setups.

Once again, sorry for all this trouble. If you need any help with setting things up for a SSL test setup then I'd be glad to help with it.

[victorolinasc]

Just to let know that I find it very awkward that Ecto.Adapters.SQL.Sandbox does NOT implement a child_spec and that this fails at all... not sure we should bring this to the Ecto people.

Thanks!

[lpil]

We could always drop the pool in the supervisor- don't want to ever use a pool as that would break Rihanna.

[victorolinasc]

I'm okay with droping the pool.