Use get_config_value for Rouille configurations #109

notplants · 2022-05-12T09:31:07Z

notplants commented

2022-05-12 09:31:07 +00:00

This PR is basically the minimum change to use the new get_config_value for "STANDALONE_MODE", "DISABLE_AUTH", "ADDR", "PORT"

I changed the name of CONFIG to ROUILLE_CONFIG, to hopefully make it more clear that ROUILLE_CONFIG is actually just a subset of the total configurations (and is ultimately getting it from the same source).

Alternatively we could delete peach-web/src/config.rs,
and change all the specific places that use a config value to call get_config_value in line. But I thought, this works, and is actually more performant (just loads these four particular config values once, on server startup), so maybe its fine.

This PR is basically the minimum change to use the new get_config_value for "STANDALONE_MODE", "DISABLE_AUTH", "ADDR", "PORT" I changed the name of CONFIG to ROUILLE_CONFIG, to hopefully make it more clear that ROUILLE_CONFIG is actually just a subset of the total configurations (and is ultimately getting it from the same source). Alternatively we could delete peach-web/src/config.rs, and change all the specific places that use a config value to call get_config_value in line. But I thought, this works, and is actually more performant (just loads these four particular config values once, on server startup), so maybe its fine.

notplants added 1 commit 2022-05-12 09:31:08 +00:00

continuous-integration/drone/pr Build is failing Details

288941e8a3 Change Rouille to use get_config_value

notplants requested review from glyph 2022-05-12 09:31:16 +00:00

notplants commented

2022-05-12 09:35:22 +00:00

wait actually im updating this so defaults arent declared in two places -- thats a bit misleading

notplants added 1 commit 2022-05-12 09:56:11 +00:00

continuous-integration/drone/pr Build is passing Details

e041e1c7f9 Change defaults to only be defined in config_manager.rs

notplants commented

2022-05-12 10:01:10 +00:00

Ok now I've updated this PR so that default values are only declared in peach-lib::config_manager (I think this is less confusing than having a second pair of defaults also defined in peach-web for these specific four values).

Currently I use an expect in lazy_static in peach-web::main if loading RouilleConfig doesn't work -- I imagine we want some type of configuration (even if its default values) and if even default values can't load, then maybe the server can't run.

It would be unfortunate if a permissions issue on config.yml caused an illegibile error. One idea: in a separate PR, we could change line 84 of config_manager.rs so that if its unable to load config.yml from disc, it just returns the default value, instead of raising an error... I'm not totally sure which is preferrable (using a default value when there is an error loading config.yml, or bubbling up that error...)

Ok now I've updated this PR so that default values are only declared in peach-lib::config_manager (I think this is less confusing than having a second pair of defaults also defined in peach-web for these specific four values). Currently I use an expect in lazy_static in peach-web::main if loading RouilleConfig doesn't work -- I imagine we want some type of configuration (even if its default values) and if even default values can't load, then maybe the server can't run. It would be unfortunate if a permissions issue on config.yml caused an illegibile error. One idea: in a separate PR, we could change line 84 of config_manager.rs so that if its unable to load config.yml from disc, it just returns the default value, instead of raising an error... I'm not totally sure which is preferrable (using a default value when there is an error loading config.yml, or bubbling up that error...)

glyph commented

2022-05-12 10:16:01 +00:00

A couple thoughts here:

Rather name the config SERVER_CONFIG or WEBSERVER_CONFIG instead of ROUILLE_CONFIG, ie. name it after the role and not the library
Return an error if unable to load config.yml from disc (in this case I think it's better to clearly communicate the error rather than papering over it, especially since file permissions are quite a fundamental thing and outside of the code itself)

A couple thoughts here: 1) Rather name the config `SERVER_CONFIG` or `WEBSERVER_CONFIG` instead of `ROUILLE_CONFIG`, ie. name it after the role and not the library 2) Return an error if unable to load `config.yml` from disc (in this case I think it's better to clearly communicate the error rather than papering over it, especially since file permissions are quite a fundamental thing and outside of the code itself)

notplants added 1 commit 2022-05-12 10:21:32 +00:00

continuous-integration/drone/pr Build is failing Details

99fd3be4ad Change RouilleConfig to ServerConfig

notplants commented

2022-05-12 10:22:53 +00:00

cool, just pushed the change (was also thinking about these names)
makes sense, returning an error is currently what we're doing

logging/propagating errors in a way that we can give useful info to the User and to feed back to the developers I think still needs works but it outside of the scope of this PR

1. cool, just pushed the change (was also thinking about these names) 2. makes sense, returning an error is currently what we're doing logging/propagating errors in a way that we can give useful info to the User and to feed back to the developers I think still needs works but it outside of the scope of this PR

👍 1

notplants added 1 commit 2022-05-12 10:24:23 +00:00