feat: Add "coder projects create" command #246
Conversation
Codecov Report
@@ Coverage Diff @@
## main #246 +/- ##
==========================================
+ Coverage 67.24% 68.33% +1.08%
==========================================
Files 124 126 +2
Lines 6516 6898 +382
Branches 69 69
==========================================
+ Hits 4382 4714 +332
+ Misses 1712 1707 -5
- Partials 422 477 +55
Continue to review full report at Codecov.
|
kylecarbs
force-pushed
the
createproject
branch
from
8db24cf
to
c493bf9
Compare
|
|
||
| "github.com/coder/coder/cli" | ||
| "github.com/coder/coder/cli/config" | ||
| "github.com/coder/coder/codersdk" | ||
| "github.com/coder/coder/provisioner/echo" |
|
|
||
| // Validates and computes the value for parameters; setting the value on "parameterByName". | ||
| func (c *compute) injectScope(ctx context.Context, scopeParams database.GetParameterValuesByScopeParams) error { | ||
| scopedParameters, err := c.db.GetParameterValuesByScope(ctx, scopeParams) |
There was a problem hiding this comment.
I wonder if scopedParameters can be null if there is a sql.ErrNoRows, like we saw with projects?
There was a problem hiding this comment.
It can be, but in Go you can iterate over a nil array without issue!
| } | ||
| require.NoError(t, err) | ||
| // #nosec | ||
| path := filepath.Join(directory, header.Name) |
There was a problem hiding this comment.
I wonder if we should have a check here to sanitize the header.Name - making sure we can't specify paths like .. that might move up the filesystem. There might be a vulnerability where you could construct a tar 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 our provisionerd with a malicious version that uploads secrets).
Kind of a similar to the example called out here: securego/gosec#439 (comment)
There was a problem hiding this comment.
Yes, and we handle this from provisionerd. But I didn't feel it was necessary for clitest.
We should abstract the tar/untar logic out so it's in one place, then add tests for that specific functionality.
There was a problem hiding this comment.
Looks good to me overall - I had one security concern and a few small nits.
Looking forward to having this in so we can have the end-to-end flow going next week!
| ) | ||
|
|
||
| func projectCreate() *cobra.Command { | ||
| return &cobra.Command{ | ||
| var ( |
There was a problem hiding this comment.
I'd propose structuring these commands as coder <verb> <subject> similar to kubectl. 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 about kubectl is that most of the commands follow an identical pattern, so kubectl 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 where kubectl project create is a valid command but kubectl workspace create is not)
There was a problem hiding this comment.
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 and plan are the only odd ones I can think of.
There was a problem hiding this comment.
kubectl has exec and logs and a few other commands that don't follow the pattern, so we could have coder ssh <workspace> or coder plan <workspace> (if we anticipate that there will be other things we'll need to SSH into, we could also do coder ssh workspace <workspace> or similar, of course - we don't have to implement coder ssh project, since that doesn't make much sense)
Thinking about it now, one reason the coder create pattern might be helpful is because of persistent flags. kubectl has a --file flag that applies to all create verbs, so you can implement it as a persistent flag on create. On the other hand, if we have separate create leaf commands, then we'd have to make sure we consistently implement --file for everything individually
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 workspace testand expect the same visual output as
coder describe user testMaybe 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!
| } | ||
|
|
||
| _, err = runPrompt(cmd, &promptui.Prompt{ | ||
| _, err = prompt(cmd, &promptui.Prompt{ |
There was a problem hiding this comment.
is there an easy way to run this noninteractively, as in a script? for example coder 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, like npm 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)
No description provided.