LemmyNet
/
lemmy
3
2
Fork 3
Code Pull Requests 1 Releases Wiki Activity

Implement config #1

Closed
nutomic wants to merge 0 commits from nutomic:config into master
pull from: nutomic:config
merge into: LemmyNet:master
Conversation 3 Commits - Files Changed 307 +10600 -26174
nutomic commented 2019-12-15 20:58:10 +00:00
Owner
Copy Link

I didnt do a thorough test, but it compiles and runs fine. Check out settings.rs for the important stuff, everything else hasnt changed much.

Some things that might need changing (whether in this PR or later):

  • add support for seperate dev/prod config files (see my comment)
  • automatically generate custom.hjson and generate jwt_secret
  • add comments to defaults.json
  • update docs
  • update env vars in .env files etc with LEMMY_ prefix (or remove prefix requirement in code)

I also noticed that Lemmy opens a new database connection for every query, is that correct? Afaik that needs a lot of resources, so we should implement a connection pool.

I didnt do a thorough test, but it compiles and runs fine. Check out settings.rs for the important stuff, everything else hasnt changed much. Some things that might need changing (whether in this PR or later): - add support for seperate dev/prod config files (see my comment) - automatically generate custom.hjson and generate jwt_secret - add comments to defaults.json - update docs - update env vars in .env files etc with `LEMMY_` prefix (or remove prefix requirement in code) I also noticed that Lemmy opens a new database connection for every query, is that correct? Afaik that needs a lot of resources, so we should implement a connection pool.
dessalines commented 2019-12-16 00:35:29 +00:00
Owner
Copy Link

Nice, this looks really good. I'll do a review for some comments.

Nice, this looks really good. I'll do a review for some comments.
dessalines requested changes 2019-12-16 01:15:26 +00:00
dessalines left a comment
Owner
Copy Link

The rest looks good, but we might as well include both the docker and ansible stuff as part of this MR, because it breaks without it.

I think that means:

  • Replacing the .env files everywhere, in docker/dev, docker/prod`, and ansible.
  • Adding the single config file volume mounts.
The rest looks good, but we might as well include both the docker and ansible stuff as part of this MR, because it breaks without it. I think that means: - Replacing the `.env` files everywhere, in `docker/dev, `docker/prod`, and ansible. - Adding the single config file volume mounts.
server/Cargo.lock
@ -335,6 +335,14 @@ name = "adler32"
version = "1.0.3"
dessalines commented 2019-12-16 01:00:26 +00:00
Owner
Copy Link

Its good to commit the lock files, same in front end too, that way we make sure we're both getting the exact same dependencies. So you're good here.

Its good to commit the lock files, same in front end too, that way we make sure we're both getting the exact same dependencies. So you're good here.
server/config/defaults.hjson Outdated
@ -0,0 +8,4 @@
}
hostname: "rrr"
bind: "0.0.0.0"
port: 8536
dessalines commented 2019-12-16 01:02:02 +00:00
Owner
Copy Link

I was debating splitting these 3 out into a sub one, maybe called host, what do you think?

I was debating splitting these 3 out into a sub one, maybe called `host`, what do you think?
nutomic commented 2019-12-16 12:18:19 +00:00
Author
Owner
Copy Link

Hmm I would rather go for listen: "0.0.0.0:8536" instead, seems simpler to understand. Not sure if grouping them as host makes much sense.

Hmm I would rather go for `listen: "0.0.0.0:8536"` instead, seems simpler to understand. Not sure if grouping them as `host` makes much sense.
server/config/defaults.hjson Outdated
@ -0,0 +22,4 @@
smtp_server: ""
smtp_login: ""
smtp_password: ""
smtp_from_address: ""
dessalines commented 2019-12-16 01:05:13 +00:00
Owner
Copy Link

There were some checks before to make sure that smtp_server exists, and if it doesn't it will tell the user when they try a password reset, that their server admin hasn't set up email yet.

These 4 should be commented out, and in the code we have to make sure it can handle non-existent email configs.

There were some checks before to make sure that `smtp_server` exists, and if it doesn't it will tell the user when they try a password reset, that their server admin hasn't set up email yet. These 4 should be commented out, and in the code we have to make sure it can handle non-existent email configs.
server/src/api/user.rs Outdated
@ -218,3 +219,3 @@
let user_form = UserForm {
name: data.username.to_owned(),
fedi_name: Settings::get().hostname.into(),
fedi_name: Settings::get().hostname.clone(),
dessalines commented 2019-12-16 01:05:46 +00:00
Owner
Copy Link

Change this to to_owned() instead of clone. I think clone is more memory intensive.

Change this to `to_owned()` instead of clone. I think clone is more memory intensive.
server/src/feeds.rs Outdated
@ -332,3 +332,3 @@
p.community_name, community_url
))
.domain(Settings::get().hostname)
.domain(Settings::get().hostname.clone())
dessalines commented 2019-12-16 01:06:15 +00:00
Owner
Copy Link

Same here.

Same here.
server/src/lib.rs Outdated
@ -178,0 +94,4 @@
if Settings::get().email.smtp_server.is_empty() {
return Err("no_email_setup".to_string());
}
let email_config = &Settings::get().email;
dessalines commented 2019-12-16 01:12:05 +00:00
Owner
Copy Link

let email_config = Settings::get().email_config.ok_or("no_email_setup")?;

That line should be able to do an early return on a missing email config, so the is_empty isn't necessary.

I definitely prefer not having empty strings, but just missing configs if its possible.

`let email_config = Settings::get().email_config.ok_or("no_email_setup")?;` That line should be able to do an early return on a missing email config, so the `is_empty` isn't necessary. I definitely prefer not having empty strings, but just missing configs if its possible.
nutomic commented 2019-12-16 12:28:44 +00:00
Author
Owner
Copy Link

I fixed everything you mentioned, except the thing with grouping hostname/bind/port. I also rebased #2.

Btw we probably want to enable this option, but I'm too lazy to rebase again :p

I fixed everything you mentioned, except the thing with grouping hostname/bind/port. I also rebased #2. Btw we probably want to enable [this option](https://docs.rs/config/0.10.1/src/config/env.rs.html#18), but I'm too lazy to rebase again :p
dessalines commented 2019-12-17 00:15:25 +00:00
Owner
Copy Link

I'd also need the docker configs changed too as part of this, as it breaks every docker / ansible build.

I'd also need the docker configs changed too as part of this, as it breaks every docker / ansible build.
nutomic added a new dependency 2019-12-17 16:43:50 +00:00
#2 Implement SQL connection pool
nutomic added a new dependency 2019-12-17 21:37:28 +00:00
#3 Add documentation for Config and update Docker/Ansible files
nutomic closed this pull request 2019-12-28 22:09:58 +00:00
dessalines referenced this issue from a commit 2019-12-29 00:56:23 +00:00
Ansible fix try #1.
dessalines referenced this issue from a commit 2020-05-17 19:54:00 +00:00
Ansible fix try #1.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Blocks
#2 Implement SQL connection pool
LemmyNet/lemmy
#3 Add documentation for Config and update Docker/Ansible files
LemmyNet/lemmy
Reference: LemmyNet/lemmy#1
No description provided.
Delete Branch "nutomic:config"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?