PretendoNetwork / account

Pretendo account server
GNU Affero General Public License v3.0
54 stars 23 forks source link

[Feature] Better config #51

Closed jonbarrow closed 1 year ago

jonbarrow commented 1 year ago

This PR makes most config settings optional by either disabling certain features or using sensible fall-backs. Also adds the ability to load config settings from environment variables instead of a config file using a config manager now instead of using the config.json from disk everywhere

jonbarrow commented 1 year ago

It's not so much "mixing", it's using env variables as a fall back if the json config is missing entirely or only certain pieces (unless the user sets the "prefers" env variable)

This was something jvs had mentioned a while ago, allowing servers to be controlled via env variables being helpful for docker

Env variables is entirely optional

ashquarky commented 1 year ago

I agree that envvars are very helpful for docker, I just wonder if using a fallback on a var-by-var basis is a good idea, rather than "your whole config is a json, or your whole config is in env". Should a piecemeal config.json just be rejected?

See e.g. Mongo, which suggests using env vars or using a config file.

jonbarrow commented 1 year ago

Ahh I see what you mean now. Are there any downsides to doing the mixing though? Besides just some slightly more complex checks, which can be solved with some minor refactoring like you suggested? If there's no real downsides or issues to this approach then I guess my only argument is "why not"?

ashquarky commented 1 year ago

I just think it has the potential to be confusing, though I guess the only situation where someone would use envvars at all is Docker, where you can easily control whether there's a config.json or not... So yeah, if you think it's neat enough post-refactoring, then sure.

jonbarrow commented 1 year ago

The more I thought about it the more I agree that the env checks at the end were pointless. Removed them