Use clap to unify usage of command line arguments
and environment variables
Details
- Reviewers
varun michal • jon - Commits
- rCOMMd1d9a6b3be86: [Identity] Use clap for argument parsing
cd service/identity cargo run -- server --help # now shows optional arguments cargo run -- server # works as intended
Diff Detail
- Repository
- rCOMM Comm
- Branch
- barthap/identity-clap2
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
services/identity/src/config.rs | ||
---|---|---|
15–16 ↗ | (On Diff #30682) | We should probably refactor this, so that main.rs forces this before traversing into the commands. Thought about it, but this was already becoming a lengthy refactor. |
42 ↗ | (On Diff #30682) | Decided to make this a path argument. It's unusual to have the directory and the file declared separately if the file path is known. |
services/identity/src/main.rs | ||
44–49 ↗ | (On Diff #30682) | Ideally, we could just do Cli::parse() once, but wasn't sure how to do this gracefully with static lifetimes associated with the global CONFIG |
Sorry for chaotic post, had so many different ideas in between, I need to play with this myself
Technically, the Config should be named ServerConfig because it's server-specific and it causes confusion to me.
There are a few ways of avoiding Cli::parse running twice. The simplest one, not necessarily correct, is to keep Cli::parse() in another Lazy.
Another option is to use OnceCell directly for server config and initialize during parsing CLI args.
Also, to address your review concerns from one of my diffs, Lazy::force() returns &'static Config which can be returned from the load_config() function - this way we can have both a "normal" variable in main and a static constant for places where it's cumbersome to pass it deeply via args.
I was recently experimenting with something similar, ended up with D8978 + D8980, but you might want to look at its previous version in https://phab.comm.dev/D8980?vs=on&id=30438#toc
One example typed by hand:
static CLI = Lazy::new(Cli::parse); pub enum Command { // ... // not sure if Clap allows to do Server(SomeStruct) because it would be better than the ServerArgs below Server { #[command(flatten)] server_args: ServerArgs, // current content of Command::Server { ... } #[clap(ignore)] server_config: OnceCell<Config>, } } impl Cli { pub fn server_config(&self) -> &'static Config { let Command::Server { server_args, server_config } = self.command else { panic!("server_config() called outside server command code"); }; server_config.get_or_try_init(|| Config::load(server_args)).expect("failed to load server config") } } fn main() { // ... match &CLI.command { // ... // current form Commands::Server(_) => { let server_config = CLI.server_config(); // or if Commands::Server contained a tuple struct (see comment above) Commands::Server(server_args) => { let server_config = server_args.server_config(); // ...
Eventually, you can create a follow-up task and assign to me, I can take it and figure out something better 😉
services/identity/src/config.rs | ||
---|---|---|
81 ↗ | (On Diff #30682) | I'd rather panic here - it's developer's fault to call it outside server command.
|
services/identity/src/config.rs | ||
---|---|---|
42 ↗ | (On Diff #30682) | It can be of type PathBuf - it will be auto parsed by clap |
Played with this for too long and there's no single good solution for this. Also, my previous proposals appeared to work only partially, they had some problems too.
I'm starting to consider the global static CONFIG as an anti-pattern. To be fully correct, I'd probably have to introduce two separate sets of Config and Command structs: one set for clap-only and the other set for in-app use.
For configuring path for keygen / server setup, I'm going to address this in a separate diff, the keygen function changed significantly and I want to keep this diff small.
Maybe you have some better ideas, I'm open to suggestions.
I'm starting to consider the global static CONFIG as an anti-pattern.
I agree with you, when I started working on services it felt like a weird pattern.
To be fully correct, I'd probably have to introduce two separate sets of Config and Command structs: one set for clap-only and the other set for in-app use.
I think this makes sense as a pattern - one is a definition of terminal UI and one is the post-processed information that is usable for the app.
But overall I think this is fine, and probably not worth spending more time on.
services/identity/src/config.rs | ||
---|---|---|
14 ↗ | (On Diff #33987) | That's an interesting pattern! |