NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: Add suspend/active user to cli#1422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Emyrk merged 16 commits intomainfromstevenmasley/user_cli_suspend_2

May 16, 2022

Merged

feat: Add suspend/active user to cli#1422

Emyrk merged 16 commits intomainfromstevenmasley/user_cli_suspend_2

May 16, 2022

Conversation

Copy link

Member

Emyrk commentedMay 13, 2022•
edited
Loading

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

Emyrk added5 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

Copy link

codecovbot commentedMay 13, 2022•
edited
Loading

Codecov Report

Merging#1422 (2ed4249) intomain (64a8b4a) willincrease coverage by0.12%.
The diff coverage is75.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%)`	⬇️
... and32 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update64a8b4a...2ed4249. Read thecomment docs.

Emyrk added4 commits

May 13, 2022 10:26

Fix unit test

23a8191

Fix linting

02968cb

Fix linting

40bef92

Fix unit test

5e80192

Emyrk requested review fromkylecarbs andf0ssel

May 13, 2022 16:02

kylecarbs reviewed

May 13, 2022

View reviewed changes

cli/userstatus.go OutdatedShow resolvedHide resolved

codersdk/users.go Outdated

		}

		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) {

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I thought about changingUser, then you have to doclient.User(id.String()) because it can't take auuid.
I can't even make it take aStringer becausestring isn't aStringer. I hit this trying to make it "generic" in some way, and kept falling short.

Copy link

Member

kylecarbsMay 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I guess I'd rather just keepUser with the UUID and addUserByUsername then.UserByIdentifier tells us very little about what the identifier is.

codersdk/users.go OutdatedShow resolvedHide resolved

PR cleanup, better short msg

53adce9

Emyrk requested a review fromkylecarbs

May 13, 2022 18:07

Copy link

Member

ammario commentedMay 13, 2022•
edited
Loading

Thoughts on outdenting it tocoder users activate <user> andcoder users suspend <user>?

My thinking isstatus 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.

Copy link

MemberAuthor

Emyrk commentedMay 13, 2022

Thoughts on outdenting it tocoder users activate <user> andcoder users suspend <user>?
My thinking isstatus 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.

Move command outside status subgroup

2ed4249

misskniss added the V2 BETA label

May 15, 2022

misskniss added this to theV2 Beta milestone

May 15, 2022

kylecarbs reviewed

May 16, 2022

View reviewed changes

cli/userstatus.go OutdatedShow resolvedHide resolved

codersdk/users.go Outdated

		}

		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) {

Copy link

Member

kylecarbsMay 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I guess I'd rather just keepUser with the UUID and addUserByUsername then.UserByIdentifier tells us very little about what the identifier is.

Copy link

MemberAuthor

Emyrk commentedMay 16, 2022

The annoying thing aboutUserByUsername andUser, is that in the cli, it's easier to just take the input as is.

If we haveUser(id) andUserByUsername(name), then it feels weird to have the code pass thearg[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.

Copy link

Member

kylecarbs commentedMay 16, 2022

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

Copy link

MemberAuthor

Emyrk commentedMay 16, 2022

Fair enough. I don't think doinguuid.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 anany and do afmt.Sprint to get it'sString(). Super annoyingstring does not implementStringer

I'll useUser(ctx, string) and callid.String() where used now

Copy link

MemberAuthor

Emyrk commentedMay 16, 2022

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

Fix table headers

ab8e5d1

Copy link

Member

kylecarbs commentedMay 16, 2022

We could just makecodersdk.Me a string!

misskniss modified the milestones:V2 Beta,Community MVP

May 16, 2022

misskniss removed the V2 BETA label

May 16, 2022

Emyrk added4 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 fromkylecarbs

May 16, 2022 20:22

kylecarbs approved these changes

May 16, 2022

View reviewed changes

Copy link

Member

kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM. User changes are really nice!

cli/userstatus.go


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

Copy link

Member

kylecarbsMay 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The use of this verb is really nice here!

Emyrk merged commitb55d83c intomain

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

Labels

None yet

4 participants

Movatterモバイル変換

feat: Add suspend/active user to cli#1422

feat: Add suspend/active user to cli#1422

Uh oh!

Conversation

Emyrk commentedMay 13, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

codecovbot commentedMay 13, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kylecarbsMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbsMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

EmyrkMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

EmyrkMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbsMay 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ammario commentedMay 13, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Emyrk commentedMay 13, 2022

Uh oh!

Uh oh!

kylecarbsMay 16, 2022

Choose a reason for hiding this comment

Uh oh!

Emyrk commentedMay 16, 2022

Uh oh!

kylecarbs commentedMay 16, 2022

Uh oh!

Emyrk commentedMay 16, 2022

Uh oh!

Emyrk commentedMay 16, 2022

Uh oh!

kylecarbs commentedMay 16, 2022

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

kylecarbsMay 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Emyrk commentedMay 13, 2022•
edited
Loading

codecovbot commentedMay 13, 2022•
edited
Loading

ammario commentedMay 13, 2022•
edited
Loading