-
Notifications
You must be signed in to change notification settings - Fork 458
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
Api Refactor: implements BatchDeleteEntry rpc #1623
Api Refactor: implements BatchDeleteEntry rpc #1623
Conversation
pkg/server/api/entry/v1/service.go
Outdated
log.WithError(err).Error("Failed to delete entry") | ||
return &entry.BatchDeleteEntryResponse_Result{ | ||
Id: id, | ||
Status: api.CreateStatus(codes.Internal, "failed to delete entry"), |
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.
Error information is missing here
pkg/server/api/entry/v1/service.go
Outdated
func (s *Service) deleteEntry(ctx context.Context, id string) *entry.BatchDeleteEntryResponse_Result { | ||
log := rpccontext.Logger(ctx) | ||
|
||
if len(id) == 0 { |
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: == ""
is more idiomatic for checking for an empty string
pkg/server/api/entry/v1/service.go
Outdated
cEntry, err := s.ds.FetchRegistrationEntry(ctx, &datastore.FetchRegistrationEntryRequest{ | ||
EntryId: id, | ||
}) | ||
if err != nil { | ||
log.WithError(err).Error("Failed to fetch entry") | ||
return &entry.BatchDeleteEntryResponse_Result{ | ||
Id: id, | ||
Status: api.CreateStatus(codes.Internal, "failed to fetch entry: %v", err), | ||
} | ||
} | ||
if cEntry.Entry == nil { | ||
log.Error("Invalid request: entry not found") | ||
return &entry.BatchDeleteEntryResponse_Result{ | ||
Id: id, | ||
Status: api.CreateStatus(codes.NotFound, "entry not found"), | ||
} | ||
} |
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.
The datastore is already supposed to handle failing if the entry does not already exist but I think it is broken and returns a code.Unknown status. We should fix the datastore to return codes.NotFound if the entry doesn't exist.
Signed-off-by: Marcos Yacob <marcos@scytale.io>
14e93bf
to
f5b5e7b
Compare
Implement Batch delete entry rpc
Which issue this PR fixes
fixes #1602