-
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: implement bundle.BatchDeleteFederatedBundle #1607
API Refactor: implement bundle.BatchDeleteFederatedBundle #1607
Conversation
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.
Very nice! Thanks @MarcosDY.
pkg/server/api/bundle/v1/service.go
Outdated
return nil, status.Errorf(codes.Unimplemented, "method BatchDeleteFederatedBundle not implemented") | ||
log := rpccontext.Logger(ctx) | ||
|
||
if len(req.TrustDomains) == 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.
I think we should probably just return an empty response here?
pkg/server/api/bundle/v1/service.go
Outdated
log.Error("Invalid request: no possible to delete server bundle") | ||
return status.Error(codes.InvalidArgument, "no possible to delete server bundle") |
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.
log.Error("Invalid request: no possible to delete server bundle") | |
return status.Error(codes.InvalidArgument, "no possible to delete server bundle") | |
log.Error("Invalid request: removing the bundle for the server trust domain is not allowed") | |
return status.Error(codes.InvalidArgument, "removing the bundle for the server trust domain is not allowed") |
_, err = s.ds.DeleteBundle(ctx, &datastore.DeleteBundleRequest{ | ||
TrustDomainId: td.String(), | ||
// TODO: what mode must we use here? | ||
Mode: datastore.DeleteBundleRequest_RESTRICT, |
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.
Hmm. This seems like the safest option for now. I'll have to think on this.
pkg/server/api/bundle/v1/service.go
Outdated
log.WithError(err).Error("Failed to delete Federated Bundle") | ||
return status.Errorf(codes.Internal, "failed to delete Federated Bundle: %v", err) |
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.
log.WithError(err).Error("Failed to delete Federated Bundle") | |
return status.Errorf(codes.Internal, "failed to delete Federated Bundle: %v", err) | |
log.WithError(err).Error("Failed to delete federated bundle") | |
return status.Errorf(codes.Internal, "failed to delete federated bundle: %v", err) |
a3f291d
to
40050f6
Compare
@@ -511,8 +511,11 @@ func TestAppendBundle(t *testing.T) { | |||
t.Run(tt.name, func(t *testing.T) { | |||
test.logHook.Reset() | |||
test.setBundle(t, sb) | |||
// Make sure no error is set to next call |
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.
Maybe a better thing to do is to initialize a new test
for each test case instead of trying to clean up/reset state on the existing test.
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.
cleaned
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.
\o/
Signed-off-by: Marcos Yacob <marcos@scytale.io>
c388348
to
5712583
Compare
Implements bundle.BatchDeleteFederatedBundle
Which issue this PR fixes
fixes #1556