🪆 separate behavior into sub-commands, with serve being the default

[itsrainy]

Addresses #222.

The structure here is somewhat complex in order to get clap to behave how we want it to. Specifically:

• We would like behavior separated into serve and run subcommands, but if neither of those is provided the default behavior should be serve (to maintain backwards compatibility)
• Some options are only valid in serve mode and some are only valid in run mode
• Some shared options are valid in both modes
• There is a shared argument, input, that is required regardless of the mode

🍽️ set serve as the default subcommand

The way that we accomplish having a default subcommand with clap is like this:

• Make the top level command struct contain just two arguments, one for the subcommand and one for the args for whichever subcommand is the default (serve in this case)
• Add args_conflicts_with_subcommands = true to our top-level struct, so that those two arguments are mutually exclusive
• Create separate structs for our sub-command-specific arguments (ServeArgs, and RunArgs)

#[derive(Parser)]
#[command(args_conflicts_with_subcommands = true)]
struct Opts {
    #[command(subcommand)]
    pub command: Option<Commands>,

    #[command(flatten)]
    pub serve: ServeArgs,
}

#[derive(Subcommand)]
pub enum Commands {
    Serve(ServeArgs),
    Run(RunArgs),
}

#[derive(Args)]
pub struct ServeArgs {
   /* <serve-specific args> */
}

pub struct RunArgs {
 /* <run-specific args> */
}

see the example in the clap documentation and this comment for more context

🤝 specify shared arguments

Normally we would put options that are valid in both modes into our top-level Opts struct. This unfortunately doesn't work because we need to specify args_conflicts_with_subcommands in order to get the default-subcommand behavior. So to specify arguments that are valid in both modes without a ton of duplication, we can create another struct SharedArgs and include it as a flattened field on each of our subcommand args structs:

...
#[derive(Args)]
pub struct ServeArgs {
  #[command(flatten)]
  shared: SharedArgs
   /* <serve-specific args> */
}

pub struct RunArgs {
  #[command(flatten)]
  shared: SharedArgs
 /* <run-specific args> */
}

#[derive(Args)]
pub struct SharedArgs {
  /* shared args */
}

🔴 specify input as a required arg

Finally, in order to make one of our shared arguments, input, required regardless of the mode we are in we need to:

• Specify it as Option<String> rather than just String (normally this would mean an arg is optional)
• Add required = true to it.

#[derive(Args)]
pub struct SharedArgs {
    /// The path to the service's Wasm module.
    #[arg(value_parser = check_module, required = true)]
    input: Option<String>,
   /* other shared args */

see this comment for some context

📜 other things

• I merged the behavior for quiet into the verbosity flag and as a result verbosity isn't entirely backwards compatible as-is. In order to make 0 mean off, I had to bump info, debug, and trace each up a number. I'm unsure what behavior we would like here (ie. if someone using Viceroy right now is used to seeing info level logs, it would make sense to have that be the default in serve mode while also providing some way to specify off if they would like)
• I've been staring at this too long to have a good sense as to whether it's a maintenance nightmare or not. It's unfortunate to not have one clean struct that specifies the CLI behavior and documentation strings, but I think that might be a necessary tradeoff to accomplish the above. Thoughts around this are most welcome.

[acfoltzer]

Holy crap, @itsrainy. I am so impressed that you were able to find a solution here given that the documentation for this scenario is so lacking. I look forward to digging in and trying to understand the details.

Before that, though, I think you raise an excellent point here:

• I've been staring at this too long to have a good sense as to whether it's a maintenance nightmare or not. It's unfortunate to not have one clean struct that specifies the CLI behavior and documentation strings, but I think that might be a necessary tradeoff to accomplish the above. Thoughts around this are most welcome.

I'd like to tag in @Integralist here to ask about the tradeoffs of ditching backwards compatibility in Viceroy's CLI. I believe that in our docs and such, we describe using fastly compute serve rather than directly using viceroy invocations. Would it be feasible to have the fastly CLI invoke Viceroy differently based on what version of Viceroy it finds?

We'd have to manage a version transition, but that one-time pain vs the ongoing burden of maintaining such an unusual application of clap might be worth it.

[Integralist]

👋🏻

re:

Would it be feasible to have the fastly CLI invoke Viceroy differently based on what version of Viceroy it finds?

This is fine to do, my only request is that the new Viceroy release that breaks backwards compatibility be held off until we have a new CLI release that has this forking logic in place.

That said, there will be issues for users who have an older version of the Fastly CLI, as the CLI will still be trying to use the old interface for Viceroy and if the CLI downloads a more recent version of Viceroy where the interface has changed then of course the Fastly CLI users will get an error. I personally don't think that's a big deal because they can just run fastly update to get the latest Fastly CLI release with the forking logic.

[itsrainy]

Awesome! Thanks for the added context @Integralist!

@acfoltzer after spending a bit more time looking at this, I think it would be useful to be a bit more explicit about the tradeoffs. With this PR as-is, the dev experience going forward would look something like this:

• Adding a new option/flag involves adding a field and doc comment to one of these three structs, depending on if the option is specific to a subcommand or is a global/shared option. This has the additional overhead of having to consider if an arg is subcommand-specific, but we're already committing to that by introducing subcommands to begin with, so ultimately I don't think this scenario is too bad
• Any place in the code that accesses Opts will have a handle for one of the subcommand structs and will be able to access shared args using that struct's .shared() fn.
• Parsing options now requires the additional line to make the match code a bit simpler (let cmd = opts.command.unwrap_or(Commands::Serve(opts.serve));. This happens exactly once in main and then once per test, which I think is reasonable and not likely to get lost or forgotten when new tests are added.

I don't think any of the above are necessarily a problem. If we drop the need for backwards compatibility and start requiring a serve or run subcommand, we would save ourselves from the last point, but I think the other two would still be relevant.

Some things I could see possibly becoming an issue in the future are:

• If we ever want to add positional arguments to individual subcommands, I could see this getting a bit tricky or ambiguous given that we already have a shared positional argument. I can't think of a specific case where we might want to do this, but it's possible.
• We might be more sensitive to clap behavior changes? For example, if the precedence of args_conflicts_with_subcommands and required = true changes for whatever reason that would be tricky to work around. Or if the --help generation logic changes, I could see that leading to some confusion.

Given all that, I think the approach I'm leaning towards is keeping backwards compatibility for now and eventually deprecating the requirement for a "default subcommand", maybe by adding an info message to that effect before any actual breaking changes are introduced. That seems cleaner to me than adding logic to the Fastly CLI to accommodate version-specific invocations of Viceroy.

Let me know if you feel strongly about this one way or the other once you get a chance to look this over!

[acfoltzer]

@itsrainy let's go with that for now, with the aim of removing the backwards compat code sometime soon.

I appreciate you running down the specifics of the dev experience, I'm mainly worried about doing something that's off the beaten path and might not be possible in the future with clap (it seems like it only accidentally works due to the primitives they expose).

Also, because it looks so different from typical clap code, I think it'll be hard for us or other engineers to come back to this a year later and easily know what's going on. Better to make sure the code is gone by then.