Phil/flowctl subcomands #360

psFried · 2022-02-07T20:48:31Z

Description:

Adds the flowctl-rs binary, which delegates to the current flowctl Go binary for most subcommands. The eventual goal is to have this binary replace the existing flowctl as the main entrypoint for users to interact with the CLI. For now, it simply delegates a bunch of subcommands to the existing flowctl binary, and also integrates a few rust-based internal subcommands.

Workflow steps:

After a make package, there will be a new binary at .build/package/bin/flowctl-rs. This binary should be a drop-in replacement for flowctl and everything should work exactly the same, except for the name of the executable. The intent is to let it bake a little as flowctl-rs before renaming it to replace the go-based flowctl

Documentation links affected:

There's no user-facing impact at this stage, and the plan is to keep it that way.

Notes for reviewers:

Reviewing commit by commit is probably best, though there are a few refactors along the way.

This change is

This includes execution of first-party external subcomands. There are not yet any internal subcommands.

Sets up a basic rust crate that all of our rust-based CLIs can use for initializing logging and doing other cli-ish things consistently.

This was chosen as a good first target for porting over a subcommand from the Go-based flowctl to the new rust-based flowctl-rs because it's fairly simple, non-essential, and doesn't rely on any Go packages. This is intended to prove out how both internal and external subcomands can co-exist within flowctl-rs.

- Migrate schemalate from separate binaries to internal subcommands. - Handle --version in flowctl-rs, which is determined in Makefile and passed as the `FLOW_VERSION` environment variable. - Remove usages of `or_bail` in the combine subcommand, in favor of just returning an error. The rationale being that using `or_bail` would make composition of subcommands needlessly painful.

saterus

This looks good to me. I pulled it down and played around with the CLI. Seems to work exactly as expected for both Rust internal commands and Go external commands.

saterus · 2022-02-08T16:39:51Z

.cargo/config

+# This just provides a default value for FLOW_VERSION during development, so that you can compile
+# without this being set. If FLOW_VERSION is defined, then this has no effect.
+[env]
+FLOW_VERSION = "dev"


I didn't know about this. Super handy. 💯

saterus · 2022-02-08T16:41:43Z

crates/flow_cli_common/src/lib.rs

+/// Helper trait for exiting the application early if there's an error.
+pub trait OrBail<T> {
+    fn or_bail(self, message: &str) -> T;
+}


This was in flux a bit, but does not appear to be in use anymore. Do we want to keep it around for later usage, or is it something we want to discourage now?

TBH, I'm thinking we should discourage its use in favor of just returning a Result<(), anyhow::Error>. I figured I'd keep it around temporarily in case any of the other CLIs use it. I think I'll be able to delete it pretty quickly, though.

saterus · 2022-02-08T16:46:12Z

crates/flowctl/Cargo.toml

+[package]
+name = "flowctl"
+version = "0.1.0"
+edition = "2021"


General question beyond this PR: what's our minimum Rust target? I ran into trouble locally because I was still running 1.55 and 2021 Edition was stabilized in 1.56. It was easy to fix, but figured I'd ask. The CI Dockerfile installs 1.57.

I don't think we've ever really taken a position on that. I think everyone's always just defaulted to installing a relatively recent version and upgrading whenever we want to use new rust features. IMO that's probably fine for now, since nobody outside of us is really depending on our crates as libraries.

saterus · 2022-02-08T16:48:24Z

crates/flowctl/src/combine.rs

+    #[clap(long, default_value = ".")]
+    pub directory: String,
+    /// Path or URL to a JSON schema, which may include reduction annotations.
+    #[clap(long, conflicts_with_all(&["source", "collection"]), requires("key"))]


I'm curious about using conflicts_with_all here. We have 2 pairs of options that are mutually exclusive. The docs don't indicate that these pairs are linked or that you can't mix and match them. Would a subcommand enum be helpful here?

Yeah, what I'd ideally like is to express these two sets of mutually exclusive arguments as an enum. I tried using somehting like this, but couldn't get it to work with clap:

pub enum SchemaAndKeySource { Direct { schema: String, key: Vec<String>, } Build { source: String, collection: String, } }

I have a general sense that subcommands should be used for verbs, rather than for grouping arguments, which was why I went with the mutual exclusion flags. Ideally we'd be able to map those flags to different rust enums without requiring an extra subcommand, which feels like it should be doable.

I found this thread for structopt, which eventually led me to this issue, which seems like it'll eventually get us what we want. They have a proposed workaround there, too, which I can take a look at, though I'd prefer to do that in a follow on PR.

saterus · 2022-02-08T16:52:45Z

crates/flowctl/src/combine.rs

+        for coll in output.collections.iter() {
+            eprintln!("collection: {:?}", coll);
+        }
+        for coll in output.built_collections.iter() {
+            eprintln!("built_collection: {:?}", coll);
+        }


We're deliberately outputting this directly to stderr, rather than as a tracing message?

lol whoops! That was just a little println debugging that shouldn't have made it in. I'll remove it.

saterus · 2022-02-08T16:56:40Z

crates/flowctl/src/main.rs

+fn run_internal<T, F>(
+    subcommand_args: InternalSubcommandArgs<T>,
+    run_fn: F,
+) -> Result<(), anyhow::Error>
+where
+    T: clap::Args + std::fmt::Debug,
+    F: FnOnce(T) -> Result<(), anyhow::Error>,


This is nice. Subcommands can only be passed arguments that are relevant to them. 👍

saterus · 2022-02-08T16:58:30Z

crates/flowctl/tests/combine.rs

@@ -0,0 +1,105 @@
+use assert_cmd::{assert::Assert, Command};


saterus · 2022-02-08T17:02:53Z

Makefile

+${PKGDIR}/bin/flowctl-rs: ${RUSTBIN}/flowctl-rs
+	cp ${RUSTBIN}/flowctl-rs $@


I just realized that I didn't have .build/bin in my PATH, but rather that I've always been picking up flowctl from my ~/go/bin. 🙃

psFried added 6 commits February 7, 2022 15:34

Initial impl of flowctl-rs

8244fde

This includes execution of first-party external subcomands. There are not yet any internal subcommands.

Add flow_cli_common rust crate

2b23c79

Sets up a basic rust crate that all of our rust-based CLIs can use for initializing logging and doing other cli-ish things consistently.

Move external subcommand to external mod

bee927a

Add flowctl-rs to package targets

60671ac

psFried requested review from saterus and jgraettinger February 7, 2022 20:48

saterus approved these changes Feb 8, 2022

View reviewed changes

remove extra printlns from flowctl-rs combine

92552b2

psFried merged commit abda1bd into master Feb 9, 2022

psFried deleted the phil/flowctl-subcomands branch February 9, 2022 15:04

oliviamiannone added the docs complete / NA No (more) doc work related to this PR label Feb 15, 2022

psFried mentioned this pull request Feb 16, 2022

Finish flowctl-rs -> flowctl transition #375

Closed

psFried mentioned this pull request Mar 1, 2022

Rename flowctl-rs -> flowctl -> flowctl-go #384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phil/flowctl subcomands #360

Phil/flowctl subcomands #360

psFried commented Feb 7, 2022 •

edited by jgraettinger

Loading

saterus left a comment

saterus Feb 8, 2022

saterus Feb 8, 2022

psFried Feb 8, 2022

saterus Feb 8, 2022

psFried Feb 8, 2022

saterus Feb 8, 2022

psFried Feb 8, 2022

saterus Feb 8, 2022

psFried Feb 8, 2022

saterus Feb 8, 2022

saterus Feb 8, 2022

saterus Feb 8, 2022

		${PKGDIR}/bin/flowctl-rs: ${RUSTBIN}/flowctl-rs
		cp ${RUSTBIN}/flowctl-rs $@

Phil/flowctl subcomands #360

Phil/flowctl subcomands #360

Conversation

psFried commented Feb 7, 2022 • edited by jgraettinger Loading

saterus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psFried commented Feb 7, 2022 •

edited by jgraettinger

Loading