Conversation
src/cigogne/internal/database.gleam
Outdated
| @@ -86,6 +105,7 @@ fn connect( | |||
| envoy.get("DATABASE_URL") | |||
| |> result.replace_error(EnvVarUnset("DATABASE_URL")) | |||
There was a problem hiding this comment.
I think this error should never get returned due to the try_recover but it was needed to compile the code. I am not sure if there is a better way to do this.
There was a problem hiding this comment.
Maybe one way to do this would be to try to get the "DATABASE_URL" envvar. Then if it is ok -> use connection_from_url. Otherwise, try to get Postgres environment variables. If none is set, return an error similar to EnvVarUnset. Otherwise, use connection_from_config.
What do you think ?
There was a problem hiding this comment.
I can certainly implement more complex error handling here. However, should we align this with the Squirrel pattern? Specifically, we would prioritize DATABASE_URL if available, and otherwise fall back to standard Postgres environment variables or their default values.
There was a problem hiding this comment.
If we go the path of using default values instead of error if the Postgres env vars do not exist do not need the EnvVarUnset error.
|
Hello, I think this would make a good addition ! The code in itself looks good. I would maybe refactor it to avoid some code duplication. I haven't given this too much thought, I'll review more in detail later ;) |
src/cigogne/internal/database.gleam
Outdated
| let procname = process.new_name("cigogne") | ||
| let config = | ||
| pog.default_config(procname) | ||
| |> apply_if_some(user, pog.user) | ||
| |> pog.password(password) | ||
| |> apply_if_some(host, pog.host) | ||
| |> apply_if_some(port, pog.port) | ||
| |> apply_if_some(name, pog.database) | ||
| pog.start(config) | ||
| |> result.map_error(ActorStartError) | ||
| |> result.map(fn(actor) { actor.data }) | ||
| } |
There was a problem hiding this comment.
Move to a connection_from_config function
src/cigogne/internal/database.gleam
Outdated
There was a problem hiding this comment.
Move to a connection_from_config function
src/cigogne/internal/database.gleam
Outdated
| @@ -86,6 +105,7 @@ fn connect( | |||
| envoy.get("DATABASE_URL") | |||
| |> result.replace_error(EnvVarUnset("DATABASE_URL")) | |||
There was a problem hiding this comment.
Maybe one way to do this would be to try to get the "DATABASE_URL" envvar. Then if it is ok -> use connection_from_url. Otherwise, try to get Postgres environment variables. If none is set, return an error similar to EnvVarUnset. Otherwise, use connection_from_config.
What do you think ?
Billuc
left a comment
Billuc
left a comment
There was a problem hiding this comment.
I tested this locally and it works fine ! I am fine with the fact that we don't have an error if we don't have any envvar set. I think it is quite consistent with how Postgres works. However, The error message we get in case of a bad config is really not helping (Database error: CONNECTION UNAVAILABLE) ^^'
I think I will create another issue for this. |
2 participants
This is a first pass for #47. Waiting on to see if you are open to adding this feature before continuing on docs and unit tests.