-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[public-api] Implement delete token #14877
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
Conversation
/werft run 👍 started the job as gitpod-build-jp-public-api-delete.1 |
d055e2f
to
4d60dbb
Compare
Where("deleted = ?", 0). | ||
Update("deleted", 1) | ||
if db.Error != nil { | ||
return 0, fmt.Errorf("Failed to delete token: %v", db.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.
[nit] To follow Go recommended style.
return 0, fmt.Errorf("Failed to delete token: %v", db.Error) | |
return 0, fmt.Errorf("failed to delete token (ID: %s): %v", tokenID.String(), db.Error) |
We do use first letter uppercase in the API Handlers to provide a cleaner user-facing error message, but here in the db
package we can stick to standard Go practice.
Here, the suggestion also includes the Token ID in the error. This makes tracking down the entry a bit simpler and I'd generally recommend including non sensitive IDs in the errors like this
There are a couple more occurrences of the uppercase.
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.
We have lots of params checkings like code below, they are uppercase too, should we change them all? If so, it could be a follow-up PR. cc @easyCZ
if req.UserID == uuid.Nil {
return PersonalAccessToken{}, fmt.Errorf("Invalid or empty userID")
}
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.
So the only place where we "should" use Uppercase is the apiv1
handler code. As that's what ultimately returns a response to the user. Everywhere else errors
should start with lowercase as per standard go style. Sorry, this is a bit confusing. Maybe I can write a middleware which uppercases the first letter of the error to get rid of this inconcistency.
f0a8a93
to
6d0b56c
Compare
Co-authored-by: Milan Pavlik <[email protected]> Co-authored-by: mustard <[email protected]>
6d0b56c
to
e328b1f
Compare
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.
Nice work!
Description
Implements delete token
Related Issue(s)
Fixes #14613
Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh