mockgcp: Refactor errors from storage.Get #1210
Conversation
Going through one method at a time, but the code is much simplified by having storage.Get return an object that follows the GRPC design patterns, where codes.NotFound is returned when the object is not found.
|
/assign @cheftako |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -125,7 +131,7 @@ func (s *typeStorage) Get(ctx context.Context, fqn string, dest proto.Message) e | |||
|
|
|||
| existing, found := s.byKey[fqn] | |||
| if !found { | |||
| return apierrors.NewNotFound(schema.GroupResource{}, fqn) | |||
There was a problem hiding this comment.
I like concentrating these concerns in the storage layer. However I think we still have 1 of the issues I originally brought up here. Why return an explicit code.NotFound error for Get() when the key is not found but not for Update() or Delete() when the key is not found (lines 98 and 111). Why not codes.AlreadyExists for Create() at line 74?
There was a problem hiding this comment.
Ah, because I want to save tasty PR treats for tomorrow/Monday :-) But now you've gone and ruined the surprise...
|
Merged in #1221 (thanks!), closing /close |
|
@justinsb: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Going through one method at a time, but the code is much simplified by
having storage.Get return an object that follows the GRPC design
patterns, where codes.NotFound is returned when the object is not
found.