Skip to content
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

Merged
merged 2 commits into from Jun 9, 2023

Conversation

CobaltCause
Copy link
Contributor

Fixes #2945.

Currently doesn't work because

error[E0277]: the trait bound `either::Either<std::string::String, DatabaseConfig>: doku::Document` is not satisfied
 --> crates/utils/src/settings/structs.rs:7:62
  |
7 | #[derive(Debug, Deserialize, Serialize, Clone, SmartDefault, Document)]
  |                                                              ^^^^^^^^ the trait `doku::Document` is not implemented for `either::Either<std::string::String, DatabaseConfig>`

Options for fixing this:

  1. Defining our own Either type and implementing Document for it
  2. Submitting a patch upstream to support the either crate

I like the second option more.

@Nutomic
Copy link
Member

Nutomic commented Jun 7, 2023

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).

@CobaltCause
Copy link
Contributor Author

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 either!" for whatever reason. This would also mean I don't have to add stuff to doku, which I tried, and I couldn't really figure out the right way to add either support to it 😅

@CobaltCause
Copy link
Contributor Author

CobaltCause commented Jun 8, 2023

Is there some documentation somewhere that I need to update? Edit: I think CI found it ❤️

@CobaltCause CobaltCause marked this pull request as ready for review June 8, 2023 00:18
@CobaltCause
Copy link
Contributor Author

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 Parts configuration method entirely.

Copy link
Member

@dessalines dessalines left a 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

///
/// Note that [`DatabaseConnection::Uri`] should be preferred since it
/// provides greater control over how the connection is made. This merely
/// exists for backwards-compatibility.
Copy link
Member

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?

Copy link
Contributor Author

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.

@CobaltCause CobaltCause force-pushed the db-uri-in-config-file branch 3 times, most recently from aeb8db8 to 0ccafbd Compare June 8, 2023 18:57
@@ -55,28 +55,62 @@ pub struct PictrsConfig {
}

#[derive(Debug, Deserialize, Serialize, Clone, SmartDefault, Document)]
#[serde(default, deny_unknown_fields)]
Copy link
Contributor Author

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

@CobaltCause
Copy link
Contributor Author

@dessalines good catch; this change fixes that. Basically, the enum variant needs #[serde(default)] but you can't do that so I had to break it out into a struct and apply it, and now it works. Previously, serde wasn't substituting in default values on missing keys.

@CobaltCause CobaltCause force-pushed the db-uri-in-config-file branch 2 times, most recently from 25a81e0 to 1b41047 Compare June 8, 2023 19:19
Copy link
Member

@dessalines dessalines left a 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!

@CobaltCause
Copy link
Contributor Author

(That force-push was just to reword the commit to include a link to the issue I opened on doku)

@CobaltCause
Copy link
Contributor Author

Maybe we should wait to merge this until we have the results of anixe/doku#33 (comment)

@Nutomic Nutomic enabled auto-merge (squash) June 9, 2023 11:30
@Nutomic Nutomic merged commit 14c18db into LemmyNet:main Jun 9, 2023
1 check was pending
@CobaltCause
Copy link
Contributor Author

CobaltCause commented Jun 9, 2023

@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.

@CobaltCause CobaltCause mentioned this pull request Jun 10, 2023
sunaurus pushed a commit to sunaurus/lemmy that referenced this pull request Jun 23, 2023
* allow specifying db uri in config file

* succumb to a bug in doku

See <anixe/doku#33>.

(cherry picked from commit 14c18db)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

database configuration abstraction is very leaky
3 participants