Configure directory with config & clap#927
Conversation
| config = add_defaults(config, cli)?; | ||
|
|
||
| // what directory should this reside in? require explicit --config-file? ~/.config? /etc? | ||
| config = config.add_source(File::new("config.toml", FileFormat::Toml).required(false)); |
There was a problem hiding this comment.
I think we should follow the same pattern as payjoin-cli for configuration file handling:
first, check for config file in xdg-compliant default directory
fall back to current working directory if no XDG directory is found
allow --config-file flag to override both defaults when specified
This would provide a consistent user experience across the codebase and follow established conventions.
There was a problem hiding this comment.
that's definitely one option, but i'm not sure, since payjoin-directory is a service, not a command line application intended to be run by users, so conventions are a bit different
usually system services store configurations in /etc not ~/.config, and state in /var/lib, but that requires system level setup of those files & directories, so perhaps it should just be easy to override those paths and still default to xdg ones
however i think i would prefer just errorring if an explicit path is not given, at least for state storage, it's less confusing than running it without error but not knowing where any state data ended up
Pull Request Test Coverage Report for Build 17330651492Details
💛 - Coveralls |
| init_logging(); | ||
|
|
||
| let dir_port = | ||
| env::var("PJ_DIR_PORT").map_or(DEFAULT_DIR_PORT, |s| s.parse().expect("Invalid port")); |
There was a problem hiding this comment.
const DEFAULT_DIR_PORT definition may be removed
| env = "PJ_DB_HOST", | ||
| default_value = "localhost:6379", |
There was a problem hiding this comment.
Use default const or remove
There was a problem hiding this comment.
it'll removed with redis, but i didn't want to break compat with the current behavior just yet in case we need to do a release for some reason this PR should behave exactly the same as before except also support a config file and -- style options
| #[arg( | ||
| long, | ||
| env = "PJ_DIR_TIMEOUT_SECS", | ||
| default_value = "30", |
There was a problem hiding this comment.
DEFAULT_TIMEOUT_SECS.to_string() can be used here or otherwise its definition removed. I think this is self-documenting so prefer removal.
| use tracing_subscriber::filter::LevelFilter; | ||
| use tracing_subscriber::EnvFilter; | ||
|
|
||
| const DEFAULT_KEY_CONFIG_DIR: &str = "ohttp_keys"; |
There was a problem hiding this comment.
you removed this one but not those in lib.rs. The others are pub but I'm not sure they need to be.
There was a problem hiding this comment.
good catch, thanks
ish, setting environment variables is fine, but they're all in one flat namespace, and we will eventually need to have flags and args for:
so it seems to me we're starting to accumulate enough complexity that 12 factor maxxing approach can get a a bit unwieldly and a config file can be helpful, especially for manual testing (no need to make a script which is actually a config file in disguise) and from a code pov IMO it would be easier to maintain all that if it's more structured and makes use of types, just for the 4 existing options main.rs i think there's kinda excessive boilerplate |
nothingmuch
force-pushed
the
directory-config
branch
2 times, most recently
from
7943fff to
efc45ad
Compare
|
converted back to draft because of #981 dependency |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
utACK efc45ad
The other 4 commits are already in master so this may need a rebase?
nothingmuch
force-pushed
the
directory-config
branch
from
efc45ad to
8f5d0af
Compare
5 participants
This uses the same approach to configuration as payjoin-cli does, with the addition of clap's
envfeature used for backwards compatibility with existing 12-factor style configuration.Depends on #981