Neat 🎉

I wonder ifscopedParameters can benull if there is asql.ErrNoRows, like we saw with projects?

It can be, but in Go you can iterate over a nil array without issue!

I wonder if we should have a check here to sanitize theheader.Name - making sure we can't specify paths like.. that might move up the filesystem. There might be a vulnerability where you could construct atar that has a header with improper paths (like../../) - and let you write a file over an existing file.

This would be bad, for example, if a malicious actor could write over a critical config file, a binary (ie, they overwrite ourprovisionerd with a malicious version that uploads secrets).

Kind of a similar to the example called out here:securego/gosec#439 (comment)

Yes, and we handle this from provisionerd. But I didn't feel it was necessary forclitest.

We should abstract the tar/untar logic out so it's in one place, then add tests for that specific functionality.

I'd propose structuring these commands ascoder <verb> <subject> similar tokubectl. When you demoed this command to me, that was what you had typed the first time, too, suggesting that it feels more "natural" - it also reads better that way, in my opinion.

For example:coder create project

The nice thing aboutkubectl is that most of the commands follow an identical pattern, sokubectl get pods,kubectl get deployments etc feel natural. Putting the verb after makes it more likely that they will diverge, which is always unexpected (e.g. cases wherekubectl project create is a valid command butkubectl workspace create is not)

That is a significantly more human approach. I'm trying to think of edge-cases in which that won't work, but I'm yet to find any!

Maybe discoverability would be hurt? eg. adding the root verb for a bespoke action a specific resource has could get really messy. I'm not sure how frequently we'll need to do that though.ssh andplan are the only odd ones I can think of.

kubectl hasexec andlogs and a few other commands that don't follow the pattern, so we could havecoder ssh <workspace> orcoder plan <workspace> (if we anticipate that there will be other things we'll need to SSH into, we could also docoder ssh workspace <workspace> or similar, of course - we don't have to implementcoder ssh project, since that doesn't make much sense)

Thinking about it now, one reason thecoder create pattern might be helpful is because of persistent flags.kubectl has a--file flag that applies to allcreate verbs, so you can implement it as a persistent flag oncreate. On the other hand, if we have separatecreate leaf commands, then we'd have to make sure we consistently implement--file for everything individually

We can also always make these changes after merging this PR, it's not urgent, but would be nice to do it before we publish things, because then we're on the hook to maintain stability of the interface

We lose specificity and customization in output when we standardize using verbs first. eg. it'd be a bit weird for a user to run:

coder describe workspacetest

and expect the same visual output as

coder describe usertest

Maybe it's good to force us down a consistency route there though.

As you mentioned, we can polish this later. It's definitely a nice standardization forcing-function that provides consistency to our CLI experience!

is there an easy way to run this noninteractively, as in a script? for examplecoder project create abc --org Coder --create-workspace=true

I'd suggest inverting this logic and assuming noninteractive mode, unless the user types-i or--interactive, as the noninteractive mode is usually more common for CLI commands

I wanted to map out the user experience before making it non-interactive. It should work how you mentioned. With flags that allow the prompts to be bypassed.

Is defaulting to interactive bad if we detect a TTY? I'm not sure if it's expected, but it would be cool.

Yeah, I think the best argument against making it non-interactive by default is the principle of least astonishment, that most tools don't do that. But there are examples of tools that do default to doing things interactive, likenpm init - so we could just merge this as-is and see what it feels like

One thing I think would be useful, though, is to make sure we're testing a non-interactive use case, ideally as part of our test suite, since that will likely be something people will want (and that people are already doing with v1)

Entirely agree!