Conversation
| import "google/api/annotations.proto"; | ||
|
|
||
|
|
||
| message Body { |
There was a problem hiding this comment.
Theres no need for a Body message, just put these fields in theUsersPasswordRequest. In the grpc option you can put * and it will pick up the fields automatically.
See ApplicationSyncRequest for an example.
| rpc UpdatePassword(UsersPasswordRequest) returns (UserResponse) { | ||
| option (google.api.http) = { | ||
| put: "/api/v1/users/{name}/password" | ||
| body: "body" |
There was a problem hiding this comment.
body should be * after making suggested changes.
cmd/argocd/commands/users.go
Outdated
| } | ||
|
|
||
| if CurrentPassword == "" { | ||
| CurrentPassword = settings.ReadAndConfirmPassword("Current Password") |
There was a problem hiding this comment.
We should not require that the user type in their current password twice. Password confirmation should be done only for the new one.
server/users/users.go
Outdated
|
|
||
| } | ||
|
|
||
| //This Function is used to Update User Passwords \ |
There was a problem hiding this comment.
Go doc convention is to repeat the name of the function in the comment. e.g.:
// UpdatePassword updates a user's password
util/settings/settings.go
Outdated
| func ReadAndConfirmPassword(types string) string { | ||
| for { | ||
| fmt.Print("*** Enter an admin password: ") | ||
| fmt.Printf("*** Enter an %s password: ", types) |
There was a problem hiding this comment.
Lets just remove types argument and say "Enter password:" and "Confirm password"
| } | ||
| message UsersPasswordRequest { | ||
| string name = 1; | ||
| Body body = 2; |
There was a problem hiding this comment.
Remove body and have currentPassword and newPassword as fields here.
server/users/users.proto
Outdated
| string currentPassword = 2; | ||
| string newPassword = 3; | ||
| } | ||
| message UsersPasswordRequest { |
There was a problem hiding this comment.
message name should be UpdatePasswordRequest
server/users/users.proto
Outdated
| Body body = 2; | ||
| } | ||
|
|
||
| message UserResponse {} |
There was a problem hiding this comment.
message name should be UpdatePasswordResponse
|
Hello @jessesuen
but before i update the body, I wanted to explain why I did it this way. in the current setup the swagger UI shows the body and required path parameter that we would expect,
but when I use '*' I see that the name gets duplicated in the body of the request.
(also when I looked online, the documentation for using '*' mentioned "Note that when using * in the body mapping, it is not possible to have HTTP parameters, as all fields not bound by the path end in the body. This makes this option more rarely used in practice of defining REST APIs. The common usage of * is in custom methods which don't use the URL at all for transferring data." which lead me to searching for an alternative) but let me know if this should still be changed, it's a quick update. (I'll be in the mountains without connectivity until Monday night, but I can make the change then) |
|
Thanks for pointing out your reasoning.
Right, this is an unfortunate side affect of grpc-gateway.
This is the behavior we want, the only consequence is the name also gets into the body, despite already being in the path. It's more important that we get the gRPC messages and data structures in a way that we like, then swagger docs. Having an extra Body field just to make the swagger docs cleaner, unnecessarily makes the gRPC API more confusing, which will be our preferred way of interacting with the API server. |
Given that you'll be out, I'll go ahead and merge your current progress. I also realize we are missing an RBAC policy rule and check for user management, so I will pick off where you left off. |
|
Kk i have the change staged on my computer so i will push it first thing in the morning. |
Signed-off-by: Florent Monbillard <f.monbillard@gmail.com>
2 participants
closes #377