- Notifications
You must be signed in to change notification settings - Fork18
Uh oh!
There was an error while loading.Please reload this page.
Conversation
From an admin POV, this will be helpful for various reasons.
| { | ||
| name: "simple list", | ||
| command: []string{"workspaces", "ls", "--all"}, | ||
| assert: func(r result) { r.success(t) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sanity check: This isn't actually checking the output - just that the command exited with 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Essentially, yeah. This mini test framework "runs" the commands in-process and checks if the command fn returned an error.
Uh oh!
There was an error while loading.Please reload this page.
internal/cmd/workspaces.go Outdated
| } | ||
| if provider != "" { | ||
| if provider != ""|| all{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This means the--all flag only works with the--provider flag. I think that's OK for now, but it needs to be made obvious in the flag description (e.g. "requires --provider flag") and there should be a check at the top of the command to ensure--provider is set if--all is set.
$ ./coder envs ls --allfatal: resource not foundThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Doesn't it mean it worksregardless of the provider flag? If I call coder with just--all, provider is "", and the logic becomesif "" != "" || true ->if false || true ->if true. This was my intent - as an admin, I want to seeall workspaces.
But looking at the actual logic/implementation for getWorkspacesByProvider, I think your point still stands. I can fix that.
Uh oh!
There was an error while loading.Please reload this page.
This looks great, just needs a few changes and it'll be solid 😄 |
Co-authored-by: Dean Sheather <dean@deansheather.com>
internal/cmd/workspaces.go Outdated
| } else if provider != "" { | ||
| var err error | ||
| workspaces, err = getWorkspacesByProvider(ctx, client, provider, user) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| var err error | ||
| workspaces, err = getWorkspaces(ctx, client, user) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shuffled this around because it seemed like the first fetch for the user-only workspaces was ignored if providers was also passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems to work well when I checked out and ran it against our deployment
internal/cmd/workspaces.go Outdated
| if provider != "" { | ||
| var workspaces []coder.Workspace | ||
| if all { | ||
| var err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You shouldn't need thevar errs here or in the other if branches since it's already defined above, then you can condense theif err check to be underneath the if branches, so each branch is just theworkspaces, err = getX() statement.
edit: check my suggestion
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Dean Sheather <dean@deansheather.com>
From an admin POV, this will be helpful for various reasons.