Conversation
|
Initiating social contact... We're scanning your PR for issues. Stand by for comments. Lots of love, |
|
@rover-app review |
|
Hey @renatillas! This repo doesn't seem to be connected to Rover. Please double check via the Rover platform! Lots of love, |
src/pevensie/postgres.gleam
Outdated
| driver: Postgres(config, None), | ||
| connect:, | ||
| disconnect:, | ||
| disconnect: fn(driver) { Ok(driver) }, |
There was a problem hiding this comment.
I'm not sure how I feel about this. Is there not a way to disconnect anymore? Can we not stop the child process?
There was a problem hiding this comment.
Not anymore. The connect and disconnect functions of pgo have been removed: https://github.com/lpil/pog/blob/main/CHANGELOG.md#v400---2025-07-07. I can try by as you say, stopping the child process.
There was a problem hiding this comment.
The tests for pog seem to use process.send_exit to shut down the connection pool. I wonder if we should implement something similar
There was a problem hiding this comment.
Ignore this - I see you've implemented that
| /// } | ||
| /// ``` | ||
| pub fn default_config() -> PostgresConfig { | ||
| pub fn default_config(pool_name: process.Name(pog.Message)) -> PostgresConfig { |
There was a problem hiding this comment.
This change needs an update to the readme, please!
src/pevensie/postgres.gleam
Outdated
| ) | ||
| let conn = pog.connect(config) | ||
| let conn = pog.supervised(config) | ||
|
|
||
| // Start supervision tree | ||
| use _ <- result.try( | ||
| supervisor.new(supervisor.OneForOne) | ||
| |> supervisor.add(conn) | ||
| |> supervisor.start() | ||
| |> result.replace_error( | ||
| snag.Snag(issue: "Failed to start Postgres connection", cause: [ | ||
| "failed to start connection", | ||
| ]), | ||
| ), | ||
| ) | ||
|
|
||
| let conn = pog.named_connection(pool_name) | ||
| let transaction_result = | ||
| pog.transaction(conn, fn(tx) { |
There was a problem hiding this comment.
I'm not sure we need a supervised connection here. The rest of the CLI isn't running in a supervision tree, so I'm not sure how much benefit we get from it.
There was a problem hiding this comment.
Yeah after understanding better how supervision works, you are right. It does not make sense to add a supervision tree on a CLI tool with short lived processes.
|
Also, would you mind bumping the Gleam version in the gleam.toml and CI? |
| let pog_config = postgres_config_to_pog_config(config) | ||
| let conn = pog.named_connection(pog_config.pool_name) | ||
|
|
There was a problem hiding this comment.
I think we're going to run into some issues here, on second thought. The disconnect method shuts down a connection pool, but the connect method no longer creates one. That makes this upgrade a breaking change. I'm not sure how to handle this, as ideally the connection pool should be supervised, but we can't start and stop that on demand.
Do you have any ideas here?
As a bit of extra context, I'll be working on Pevensie V2 soon, which will only support Postgres (and therefore can rely on users passing in a Postgres connection directly). I'm wondering if we should just accept that it's going to be a breaking change and adopt that model, with the connect and disconnect methods effectively becoming no-ops in this case. It's not a great solution, but it would allow us to get something working.
There was a problem hiding this comment.
I think we should leave the connect and disconnect methods behind, as you said. It's a breaking change but migrations shouldn't be hard.
2 participants
This Draft aims to bump the dependencies of pog, gleam_otp, gleam_erlang and has the addition of gleam_time as it's used by pog now.