-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add change-password admin command #1304
Conversation
cmd/admin.go
Outdated
|
||
subcmdChangePassword = cli.Command{ | ||
Name: "change-password", | ||
Usage: "Print encoded version of a given password", |
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.
It's not being printed.
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.
I meant, that the encoded password isn't being printed. Should the usage not just be "change password of a user" ?
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.
Ah, of course, will fix and squash
It is now:
$ ./gitea admin --help | grep change
change-password Print encoded version of a given password
|
LGTM |
Yes, much better. And you're right about giving an error for users where it wouldn't be effective or some other method may still allow them to log in (if trying to disable with password change) |
@@ -57,8 +58,59 @@ to make automatic initialization process more smoothly`, | |||
}, | |||
}, | |||
} | |||
|
|||
subcmdChangePassword = cli.Command{ | |||
Name: "change-password", |
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.
Why not just password
?
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.
No reason, this was suggested by @bkcsoft on IRC if I recall correctly, and I thought it was a clean and self-describing name
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.
change-password
is too long for a command, I think password
is better!
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.
vote password
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.
LGTM |
You also have to type --username and --password so it is balanced to me
--strk;
Sent from hand-held device, sorry for brevity
|
cmd/admin.go
Outdated
Action: runChangePassword, | ||
Flags: []cli.Flag{ | ||
cli.StringFlag{ | ||
Name: "username", |
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.
support -u
or --username
. Maybe username,u
cmd/admin.go
Outdated
Usage: "Username", | ||
}, | ||
cli.StringFlag{ | ||
Name: "password", |
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.
support -p
or --password
. Maybe password,p
agree with @appleboy |
This PR has 2 LGTM, please merge |
@strk Do you see my suggestions about adding more flags? |
@appleboy added -u and -p options, and rebased |
so I'm also happy with just "passwd" as the admin name (to reflect the unix command) but as the only existing command is |
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.
Generally I would prefer a user sub-command instead of admin.
cmd/admin.go
Outdated
cli.StringFlag{ | ||
Name: "username,u", | ||
Value: "", | ||
Usage: "Username", |
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.
Pretty useless usage information.
cmd/admin.go
Outdated
cli.StringFlag{ | ||
Name: "password,p", | ||
Value: "", | ||
Usage: "Password", |
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.
Pretty useless usage information.
If every single maintainer complains about something this will become much slower than Gogs. It's already happening that Gogs is ahead. This PR got 2 positive reviews, so about the rules it should just be merged. If you think maintainers should also have veto powers so please add a section about that in the CONTRIBUTING.md file. |
@tboerger don't really see the point of having |
LGTM working? |
LGTM is working but waiting build status. |
@bkcsoft Thanks for improving usage information |
Not working yet, need help