feat: Add suspend/active user to cli by Emyrk · Pull Request #1422 · coder/coder

Emyrk · 2022-05-13T15:00:04Z

coder users activate <username | user_id>
coder users suspend <username | user_id>

codecov · 2022-05-13T15:07:42Z

Codecov Report

Merging #1422 (2ed4249) into main (64a8b4a) will increase coverage by 0.12%.
The diff coverage is 75.57%.

@@            Coverage Diff             @@
##             main    #1422      +/-   ##
==========================================
+ Coverage   67.07%   67.19%   +0.12%     
==========================================
  Files         287      292       +5     
  Lines       19307    19612     +305     
  Branches      241      258      +17     
==========================================
+ Hits        12950    13179     +229     
- Misses       5016     5079      +63     
- Partials     1341     1354      +13

Flag	Coverage Δ
unittest-go-macos-latest	`54.42% <75.57%> (+0.35%)`	⬆️
unittest-go-postgres-	`65.71% <75.57%> (+0.23%)`	⬆️
unittest-go-ubuntu-latest	`56.80% <75.57%> (+0.17%)`	⬆️
unittest-go-windows-2022	`52.78% <75.57%> (+0.37%)`	⬆️
unittest-js	`74.73% <ø> (+0.08%)`	⬆️

Impacted Files	Coverage Δ
coderd/users.go	`61.80% <57.14%> (+0.36%)`	⬆️
cli/userstatus.go	`70.00% <70.00%> (ø)`
codersdk/users.go	`65.12% <84.61%> (+1.33%)`	⬆️
cli/userlist.go	`71.42% <100.00%> (-11.43%)`	⬇️
cli/users.go	`100.00% <100.00%> (ø)`
coderd/coderd.go	`94.65% <100.00%> (+0.06%)`	⬆️
cli/autostart.go	`66.37% <0.00%> (-8.92%)`	⬇️
cli/autostop.go	`66.37% <0.00%> (-8.63%)`	⬇️
peerbroker/dial.go	`77.04% <0.00%> (-6.56%)`	⬇️
site/src/components/Section/Section.tsx	`61.11% <0.00%> (-5.56%)`	⬇️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64a8b4a...2ed4249. Read the comment docs.

kylecarbs · 2022-05-13T16:09:55Z


-func (c *Client) userByIdentifier(ctx context.Context, ident string) (User, error) {
+// UserByIdentifier returns a user for the username or uuid provided.
+func (c *Client) UserByIdentifier(ctx context.Context, ident string) (User, error) {


Should we change User to be this instead of creating a new handler? It seems like User is just wrong right now.

For all /users/<param> routes, we could create a generic that accepts a string or UUID. Right now it's incorrectly stated that they require UUIDs.

I tried to make a generic, but unfortunately you cannot make struct methods generics 😢

I thought about changing User, then you have to do client.User(id.String()) because it can't take a uuid.
I can't even make it take a Stringer because string isn't a Stringer. I hit this trying to make it "generic" in some way, and kept falling short.

I guess I'd rather just keep User with the UUID and add UserByUsername then. UserByIdentifier tells us very little about what the identifier is.

ammario · 2022-05-13T19:17:30Z

Thoughts on outdenting it to coder users activate <user> and coder users suspend <user>?

My thinking is status is a broad term and it's not immediately obvious it has to do with activation. For example, one may think it has to do with roles or email verification.

Emyrk · 2022-05-13T20:04:33Z

Thoughts on outdenting it to coder users activate <user> and coder users suspend <user>?

My thinking is status is a broad term and it's not immediately obvious it has to do with activation. For example, one may think it has to do with roles or email verification.

I'm cool with that. I had that originally, but don't have a strong opinion either way.

kylecarbs · 2022-05-16T15:37:00Z


-func (c *Client) userByIdentifier(ctx context.Context, ident string) (User, error) {
+// UserByIdentifier returns a user for the username or uuid provided.
+func (c *Client) UserByIdentifier(ctx context.Context, ident string) (User, error) {


I guess I'd rather just keep User with the UUID and add UserByUsername then. UserByIdentifier tells us very little about what the identifier is.

Emyrk · 2022-05-16T15:49:57Z

The annoying thing about UserByUsername and User, is that in the cli, it's easier to just take the input as is.

If we have User(id) and UserByUsername(name), then it feels weird to have the code pass the arg[0] which can be either a uuid, or a username.

The appropriate code would be to try and parse the arg as a uuid, and if it fails, try as a username. But the api already does that, and it seems excessive to check locally as well.

So having a function accept either is useful when just passing input along.

kylecarbs · 2022-05-16T15:52:28Z

Fair enough. I don't think doing uuid.String() is bad. It represents what we do internally anyways. Having multiple methods that do the exact same thing seems worse.

Emyrk · 2022-05-16T15:56:55Z

Fair enough. I don't think doing uuid.String() is bad. It represents what we do internally anyways. Having multiple methods that do the exact same thing seems worse.

The nasty thing would be to accept an any and do a fmt.Sprint to get it's String(). Super annoying string does not implement Stringer

I'll use User(ctx, string) and call id.String() where used now

Emyrk · 2022-05-16T15:57:44Z

This is werid now client.User(cmd.Context(), codersdk.Me.String()). :(

kylecarbs · 2022-05-16T19:33:26Z

We could just make codersdk.Me a string!

…_suspend_2

kylecarbs

LGTM. User changes are really nice!

kylecarbs · 2022-05-16T20:25:56Z

+
+			// Prompt to confirm the action
+			_, err = cliui.Prompt(cmd, cliui.PromptOptions{
+				Text:      fmt.Sprintf("Are you sure you want to %s this user?", verb),


The use of this verb is really nice here!

* feat: Add suspend/active user to cli * UserID is now a string and allows for username too

Emyrk added 5 commits May 13, 2022 09:57

feat: Add suspend/active user to cli

1bed8c8

Add aliases

ddf2571

Add comment to displayUsers

77f4890

Add comment to displayUsers

f57008c

Import order

6989e13

Emyrk added 4 commits May 13, 2022 10:26

Fix unit test

23a8191

Fix linting

02968cb

Fix linting

40bef92

Fix unit test

5e80192

Emyrk requested review from f0ssel and kylecarbs May 13, 2022 16:02

kylecarbs reviewed May 13, 2022

View reviewed changes

PR cleanup, better short msg

53adce9

Emyrk requested a review from kylecarbs May 13, 2022 18:07

Move command outside status subgroup

2ed4249

misskniss added the V2 BETA label May 15, 2022

misskniss added this to the V2 Beta milestone May 15, 2022

kylecarbs reviewed May 16, 2022

View reviewed changes

Fix table headers

ab8e5d1

misskniss modified the milestones: V2 Beta, Community MVP May 16, 2022

misskniss removed the V2 BETA label May 16, 2022

Emyrk added 4 commits May 16, 2022 15:04

UserID is now a string and allows for username too

e19b3bb

rename args

c36a787

Merge remote-tracking branch 'origin/main' into stevenmasley/user_cli…

f062a23

…_suspend_2

Fix tests

c228923

Emyrk requested a review from kylecarbs May 16, 2022 20:22

kylecarbs approved these changes May 16, 2022

View reviewed changes

Emyrk merged commit b55d83c into main May 16, 2022

Emyrk deleted the stevenmasley/user_cli_suspend_2 branch May 16, 2022 20:29

kylecarbs pushed a commit that referenced this pull request Jun 10, 2022

feat: Add suspend/active user to cli (#1422)

b76740a

* feat: Add suspend/active user to cli * UserID is now a string and allows for username too

Conversation

Emyrk commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kylecarbs May 13, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbs May 13, 2022

Choose a reason for hiding this comment

Uh oh!

Emyrk May 13, 2022

Choose a reason for hiding this comment

Uh oh!

Emyrk May 13, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbs May 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ammario commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Emyrk commented May 13, 2022

Uh oh!

Uh oh!

kylecarbs May 16, 2022

Choose a reason for hiding this comment

Uh oh!

Emyrk commented May 16, 2022

Uh oh!

kylecarbs commented May 16, 2022

Uh oh!

Emyrk commented May 16, 2022

Uh oh!

Emyrk commented May 16, 2022

Uh oh!

kylecarbs commented May 16, 2022

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

kylecarbs May 16, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Emyrk commented May 13, 2022 •

edited

Loading

codecov Bot commented May 13, 2022 •

edited

Loading

ammario commented May 13, 2022 •

edited

Loading