feat!: explicit binary directory for postgres#22
Conversation
lf-
force-pushed
the
jade/push-wkzwklynsnuy
branch
from
9893b8a to
b77fca9
Compare
Motivation: we're integrating tmp-postgrust into our Buck2 builds of https://github.com/mercurytechnologies/locally-euclidean, which, in the Bazel tradition, looks dimly on resolution of executables from PATH (in fact, there's not any easy way to put something *extra* into the PATH of a target, and this friction is intentional). This PR adds a field to TmpPostgrustFactoryConfig to explicitly give the postgres bin directory. This is a breaking change, the resolution for existing users is to change their initialization to create with `Default::default()`, then modify the fields they care about. Perhaps we should have a builder? idk. However, this will be the last time that API breaks in the future, since we add a `#[non_exhaustive]` to forbid consumers from writing code that breaks when we add fields. This also fixes a bug where `createdb` and `createuser` only considered PATH and not the fallback directories. Disclosure: the code here was written with Claude, but I've reviewed it fairly carefully and added some extra clarification at some points that benefit from it. The commit message is handwritten because I have standards. The approach here is exactly what I would write by hand.
lf-
force-pushed
the
jade/push-wkzwklynsnuy
branch
from
b77fca9 to
ba07fab
Compare
|
Okay, now that I've tested it with my downstream project, I feel confident to send this for review :) Let me know if we should add a builder or something. |
johnchildren
left a comment
johnchildren
left a comment
There was a problem hiding this comment.
Thank you for the contribution, looks good 
!
| >, | ||
| { | ||
| let process = self.start_postgresql(&self.cache_dir)?; | ||
| let process = self.start_postgresql_async(&self.cache_dir).await?; |
There was a problem hiding this comment.
Ah interesting catch, I guess I missed this before
There was a problem hiding this comment.
claude review stays winning on that one, it got caught while checking my work on the rest of it :)
|
Re: breaking the API for the config, I think it's fine with a version bump, I only introduced the config option in the last release and there was only one option before. I agree that a builder API would be better longer term so I can just make an issue for that to be done later. |
|
#23 issue for using the builder pattern |
2 participants
Motivation: we're integrating tmp-postgrust into our Buck2 builds of https://github.com/mercurytechnologies/locally-euclidean, which, in the Bazel tradition, looks dimly on resolution of executables from PATH (in fact, there's not any easy way to put something extra into the PATH of a target, and this friction is intentional).
This PR adds a field to TmpPostgrustFactoryConfig to explicitly give the postgres bin directory.
This is a breaking change, the resolution for existing users is to
change their initialization to create with
Default::default(), thenmodify the fields they care about. Perhaps we should have a builder?
idk.
However, this will be the last time that API breaks in the future, since we
add a
#[non_exhaustive]to forbid consumers from writing code thatbreaks when we add fields.
This also fixes a bug where
createdbandcreateuseronly considered PATH and not the fallback directories.Disclosure: the code here was written with Claude, but I've reviewed it fairly carefully and added some extra clarification at some points that benefit from it. The commit message is handwritten because I have standards. The approach here is exactly what I would write by hand.