-
Notifications
You must be signed in to change notification settings - Fork 3
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
Detail and delete api key tenant handlers #116
Conversation
This pull request has been linked to Shortcut Story #10411: Implement APIKey Resource Detail/Delete Handlers. |
Modified: key.Modified.Format(time.RFC3339Nano), | ||
ID: key.ID.String(), | ||
ClientID: key.ClientID, | ||
Name: key.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.
List is only supposed to return some of the fields (see the quarterdeck implementation) so I've updated the handler to reflect that.
|
||
// Delete the API key using Quarterdeck | ||
// TODO: Handle error status codes returned by Quarterdeck | ||
if err = s.quarterdeck.APIKeyDelete(ctx, apiKeyID); err != nil { |
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.
Note that quarterdeck will actually return 404 if the API key is not in the user's organization (to prevent users deleting keys from other organizations). However, we should probably be handling the different error cases that get returned from Quarterdeck so I've added some TODOs in this file.
pkg/tenant/apikeys.go
Outdated
return | ||
} | ||
|
||
c.JSON(http.StatusNoContent, nil) |
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.
Could this be replaced with c.Status(http.StatusNoContent)
or does nil need to be returned here?
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.
Yes, thanks for catching this!
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.
Thank you for providing comments in the PR! I have one small question, but this is good to go.
Scope of changes
This implements the detail and delete api key tenant handlers. These should be pretty straightforward given the pattern setup in SC-10407.
There was also a slight change to
ProjectAPIKeyList
which should also be reviewed.Fixes SC-10411
Type of change
Acceptance criteria
The endpoint handlers make sense given the TODOs in the code.
Author checklist
Reviewer(s) checklist