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

Emi · 2021-10-30T16:19:43Z

Emi commented

2021-10-30 16:19:43 +00:00

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

[web] Add the ability to parse arguments 498cbc4fce

(don't actually do anything with them yet)

Merge branch 'main' into configuration 5087d4738d

[web] Add support for reading from a configuration file 7f3b9ef2ec

[lib] Implement serializing/deserializing UserPreferences 7fb9ed0119

[lib] Impl FromStr for UserPreferences c4b8b55414

[web] Properly parse prefstring in CLI args 6e1afdb4b6

[web] Respect config settings where possible 37c0390678

[lib] Add PronounList struct for serializing/deserializing lists of pronouns 1e78a528ce

[web] Add support for specifying pronouns via the command line 2aeffefee5

Emi added 1 commit 2021-10-30 16:24:05 +00:00

[web] Shit I forgot to attach the default config file 58d663c45f

sashanoraa requested changes 2021-10-30 17:40:28 +00:00

sashanoraa left a comment

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

src/user_preferences/mod.rs Outdated

					
				@ -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)]

sashanoraa commented

2021-10-30 17:22:59 +00:00

Import Serialize and Deserialize directly instead of qualifing.

Emi commented

2021-10-30 17:56:12 +00:00

Resolved in 6d087bd

Resolved in 6d087bd

Emi marked this conversation as resolved

web/src/configuration.rs Outdated

					
				@ -0,0 +54,4 @@

								   short = 'c'))]

					/// path to the config file (generated if needed).  Defaults to

					/// /etc/pronouns_today.yml

					pub config: String,

sashanoraa commented

2021-10-30 17:28:52 +00:00

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

Emi commented

2021-10-30 17:56:36 +00:00

Resolved in 6d087bd

Resolved in 6d087bd

Emi marked this conversation as resolved

web/src/configuration.rs

					
				@ -0,0 +63,4 @@

					#[argh(option, short = 'p')]

					/// the port to listen on

					pub port: Option<u16>,

sashanoraa commented

2021-10-30 17:30:00 +00:00

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

Emi commented

2021-10-30 18:09:34 +00:00

Resolved in c6ba724

Resolved in c6ba724

Emi marked this conversation as resolved

web/src/configuration.rs

					
				@ -0,0 +88,4 @@

							match File::open(&self.config) {

								Ok(raw_cfg) => {

									serde_yaml::from_reader(raw_cfg)

										.map_err(ConfigError::MalformedConfig)

sashanoraa commented

2021-10-30 17:40:26 +00:00

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

Emi commented

2021-10-30 17:46:39 +00:00

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

Emi marked this conversation as resolved

web/src/configuration.rs

					
				@ -0,0 +120,4 @@

					pub instance_settings: InstanceSettings,

					/// The port for the server to bind to.  Defaults to 1312

					pub port: u16,

sashanoraa commented

2021-10-30 17:26:11 +00:00

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

Emi commented

2021-10-30 17:45:06 +00:00

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

sashanoraa commented

2021-10-30 17:36:31 +00:00

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

Emi commented

2021-10-30 17:56:53 +00:00

Resolved in 6d087bd

Resolved in 6d087bd

Emi marked this conversation as resolved

Emi added 1 commit 2021-10-30 17:55:56 +00:00

[web+lib] Apply some recommendations by @zethra 6d087bd155

- #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

Allow specifying a socket address c6ba724434