Page MenuHomePhabricator

[Identity] Use clap for argument parsing
ClosedPublic

Authored by bartek on Aug 31 2023, 8:10 PM.
Tags
None
Referenced Files
F3342009: D9058.diff
Fri, Nov 22, 12:11 AM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:49 PM
Unknown Object (File)
Sun, Oct 27, 4:45 PM
Unknown Object (File)
Sep 27 2024, 1:16 PM
Subscribers

Details

Summary

Use clap to unify usage of command line arguments
and environment variables

https://linear.app/comm/issue/ENG-4674

Test Plan
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.
My rule is:

  • Results are for code that can fail due to users actions or external causes in runtime
  • Panics are for developer errors
services/identity/src/config.rs
42 ↗(On Diff #30682)

It can be of type PathBuf - it will be auto parsed by clap

bartek edited reviewers, added: jon; removed: bartek.

Taking this one, I'll play with this a bit

bartek edited reviewers, added: michal; removed: jon.

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.

Update rust docker image, looks like clap features require new Rust

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!

This revision is now accepted and ready to land.Nov 29 2023, 4:56 AM
This revision was automatically updated to reflect the committed changes.