-
-
Notifications
You must be signed in to change notification settings - Fork 885
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
allow specifying db uri in config file #2956
Conversation
I dont think it makes sense to pull in a dependency for something that can be implemented with a simple enum (unless Im missing something). |
Yeah, I was actually thinking about this a while after opening this PR while doing other stuff, and you're totally right. Something in my brain is just hard wired to go "Oh you want X xor Y? Better pull in |
bd80d9a
to
34c794e
Compare
Is there some documentation somewhere that I need to update? Edit: I think CI found it ❤️ |
Also, seems like a breaking release is about to happen. I don't know how y'all feel about doing breaking configuration changes, but this might be a good time to do one and just get rid of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting this error with current lemmy.hjson setups:
thread 'main' panicked at 'Failed to load settings file, see documentation (https://join-lemmy.org/docs/en/administration/configuration.html):
LemmyError { message: None, inner: "data did not match any variant of untagged enum DatabaseConnection" near 15:3, context: SpanTrace [] }',
crates/utils/src/settings/mod.rs:20:20
crates/utils/src/settings/structs.rs
Outdated
/// | ||
/// Note that [`DatabaseConnection::Uri`] should be preferred since it | ||
/// provides greater control over how the connection is made. This merely | ||
/// exists for backwards-compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not convinced that this should be removed, it seems much easier for users who are unfamiliar with postgres. Why not keep both options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue it would be best to remove Parts
because:
- It's less flexible than
Uri
- Lemmy already has an environment variable for configuring the database via a connection URI, so there is already precedent for supporting connection URIs in Lemmy
- People should know how to write/use connection URIs anyway since they're going to need to know how to administrate PostgreSQL anyway, and most tools for this (e.g.
psql
) accept URIs - It will reduce the amount of "why can't I use the unix domain socket with the
Parts
configuration?" questions you'll get, since that's how I initially ran into this - I'd guess that most smaller instances will have Lemmy and PostgreSQL running on the same machine, and
Uri
will allow them to use peer authentication which will save them the hassle of managing additional secrets
I'll change the doc comment to link to PostgreSQL's documentation for writing connection URIs though, which should help with the familiarity problem.
aeb8db8
to
0ccafbd
Compare
@@ -55,28 +55,62 @@ pub struct PictrsConfig { | |||
} | |||
|
|||
#[derive(Debug, Deserialize, Serialize, Clone, SmartDefault, Document)] | |||
#[serde(default, deny_unknown_fields)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use deny_unknown_fields
in combination with flatten
: https://serde.rs/container-attrs.html#deny_unknown_fields
0ccafbd
to
fef1123
Compare
@dessalines good catch; this change fixes that. Basically, the enum variant needs |
25a81e0
to
1b41047
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmk I tested this on an existing install, and it worked well. Once those CI issues get solved, I'm good to merge this. Thanks!
374cb4c
to
6dfb278
Compare
(That force-push was just to reword the commit to include a link to the issue I opened on doku) |
Maybe we should wait to merge this until we have the results of anixe/doku#33 (comment) |
6dfb278
to
721c638
Compare
@Nutomic: this PR didn't include the new doku release and so the generated documentation is still malformed. Don't forget to update doku and regenerate it to fix that. Edit: I see this hasn't been done yet so I opened a PR. Presumably you've got bigger fish to fry. |
* allow specifying db uri in config file * succumb to a bug in doku See <anixe/doku#33>. (cherry picked from commit 14c18db)
Fixes #2945.
Currently doesn't work because
Options for fixing this:
Either
type and implementingDocument
for iteither
crateI like the second option more.