[web] Support runtime configuration using commandline args + config file #1

Manually merged
Emi merged 0 commits from configuration into main 2021-10-30 18:19:34 +00:00
Owner
  • Currently supports changing the port and pronoun list.
  • Can read from a config file and also apply overrides from the command line.
  • Will generate a config file if missing
  • Uses two sub-commands (run and dump-statics [not yet implemented])
  • Can parse pronoun lists and prefstrings and handle errors appropriately
  • prefstring implementation not yet complete (will be done on a seperate branch, after merge)
  • Includes several [lib] changes to enable extra serialization/parsing functionality
- Currently supports changing the port and pronoun list. - Can read from a config file and also apply overrides from the command line. - Will generate a config file if missing - Uses two sub-commands (`run` and `dump-statics` [not yet implemented]) - Can parse pronoun lists and prefstrings and handle errors appropriately - prefstring implementation not yet complete (will be done on a seperate branch, after merge) - Includes several `[lib]` changes to enable extra serialization/parsing functionality
Emi added 9 commits 2021-10-30 16:19:44 +00:00
Emi added 1 commit 2021-10-30 16:24:05 +00:00
sashanoraa requested changes 2021-10-30 17:40:28 +00:00
sashanoraa left a comment
Collaborator

I've left comments on things I think need changing.

I've left comments on things I think need changing.
@ -26,3 +29,3 @@
/// `UserPreferences` struct shares a lifetime with the settings it was created with.
#[non_exhaustive]
#[derive(Debug, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
Collaborator

Import Serialize and Deserialize directly instead of qualifing.

Import Serialize and Deserialize directly instead of qualifing.
Author
Owner

Resolved in 6d087bd

Resolved in 6d087bd
Emi marked this conversation as resolved
@ -0,0 +54,4 @@
short = 'c'))]
/// path to the config file (generated if needed). Defaults to
/// /etc/pronouns_today.yml
pub config: String,
Collaborator

Don't use Strings for paths. Use PathBuf instead.

Don't use Strings for paths. Use PathBuf instead.
Author
Owner

Resolved in 6d087bd

Resolved in 6d087bd
Emi marked this conversation as resolved
@ -0,0 +63,4 @@
#[argh(option, short = 'p')]
/// the port to listen on
pub port: Option<u16>,
Collaborator

The user should be able to specify a full socket address.

The user should be able to specify a full socket address.
Author
Owner

Resolved in c6ba724

Resolved in c6ba724
Emi marked this conversation as resolved
@ -0,0 +88,4 @@
match File::open(&self.config) {
Ok(raw_cfg) => {
serde_yaml::from_reader(raw_cfg)
.map_err(ConfigError::MalformedConfig)
Collaborator

This should maybe contain the serde error so we can print what was wrong in the config file.

This should maybe contain the serde error so we can print what was wrong in the config file.
Author
Owner

A little confused here? This does wrap the serde error. See line 156

A little confused here? This does wrap the serde error. See line 156
Emi marked this conversation as resolved
@ -0,0 +120,4 @@
pub instance_settings: InstanceSettings,
/// The port for the server to bind to. Defaults to 1312
pub port: u16,
Collaborator

Config should specify the full socket address, not just the port. It should also support multiple.

Config should specify the full socket address, not just the port. It should also support multiple.
Author
Owner

I think binding to multiple addresses might be a little bit beyond the scope of this PR at least. I'll add the ability to specify binding to a specific address though. I still think that even that might be slightly beyond scope of something that's primarily going to be used in docker containers and/or behind reverse proxies, but it's easy enough to add

I think binding to multiple addresses might be a little bit beyond the scope of this PR at least. I'll add the ability to specify binding to a specific address though. I still think that even that might be slightly beyond scope of something that's primarily going to be used in docker containers and/or behind reverse proxies, but it's easy enough to add
web/src/main.rs Outdated
@ -145,0 +167,4 @@
println!("A config file has been generated at {}! Please check it out
and modify it to your liking, and then run this command
again", path);
exit(0);
Collaborator

Maybe return an error here since there server didn't actually start.

Maybe return an error here since there server didn't actually start.
Author
Owner

Resolved in 6d087bd

Resolved in 6d087bd
Emi marked this conversation as resolved
Emi added 1 commit 2021-10-30 17:55:56 +00:00
6d087bd155
[web+lib] Apply some recommendations by @zethra
- #1 (comment)
  Import Serialize and Deserialize directly instead of qualifing.

- #1 (comment)
  Don't use Strings for paths. Use PathBuf instead.

- #1 (comment)
  Maybe return an error here since there server didn't actually start.
Emi added 1 commit 2021-10-30 18:09:21 +00:00
Emi added 1 commit 2021-10-30 18:13:22 +00:00
Emi manually merged commit 26da64922d into main 2021-10-30 18:19:34 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
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.

Dependencies

No dependencies set.

Reference: Emi/PronounsToday#1
No description provided.