Adding protocol.Result to Send and Request client interface. #425
Conversation
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
| resp, err := c.Request(ctx, e) | ||
| if err != nil { | ||
| log.Printf("failed to send: %v", err) | ||
| resp, result := c.Request(ctx, e) |
There was a problem hiding this comment.
Most of the samples that use client and assume Send/Request throw errors for non nil results need to be updated.
Signed-off-by: Scott Nichols <snichols@vmware.com>
README.md
Outdated
| // handle result as an accepted event. | ||
| } else if cloudevents.IsNACK(result) { | ||
| // handle result as a rejected event. | ||
| } else { |
There was a problem hiding this comment.
Mh why that case should be available?
There was a problem hiding this comment.
if you tried to send bad data it will not NAC nor ACK, it will throw an error
I think the last else should be else if result != nil {
I will fix
There was a problem hiding this comment.
ah ok fine, can you add a comment about it?
v2/client/client.go
Outdated
| @@ -138,18 +138,21 @@ func (c *ceClient) Request(ctx context.Context, e event.Event) (*event.Event, er | |||
| var resp *event.Event | |||
| msg, err := c.requester.Request(ctx, (*binding.EventMessage)(&e)) | |||
| defer func() { | |||
| if msg == nil { | |||
There was a problem hiding this comment.
Code style comment: IMO it looks cleaner to branch on msg != nil and both schedule the defer and convert the msg to event
v2/client/client.go
Outdated
|
|
||
| // try to turn msg into an event, it might not work and that is ok. | ||
| if rs, err := binding.ToEvent(ctx, msg); err != nil { | ||
| cecontext.LoggerFrom(ctx).Debugw("failed calling ToEvent", zap.Error(err), zap.Any("resp", msg)) |
There was a problem hiding this comment.
IMO this is not the expected behaviour. I propose this change:
- If the message is encoding == unknown, then maybe returning an error doesn't make sense (a message with encoding unknown could be just a 202 accepted, so it's not an error from the client POV but just an ack)
- If the message is a known encoding, but there was an error while translating to event, then something is wrong with the event response (invalid event), so the error should be reported back
There was a problem hiding this comment.
I had to make a choice on which error to send up for responses.
Signed-off-by: Scott Nichols <snichols@vmware.com>
NOTE: This is a breaking API change. client.Send and client.Receive might return a non-nil result which is an ACK.
use
cloudevents.IsACK(result)orprotocol.IsACK(result)to test the non-nil case.Fixes #422
We now have the hooks for protocols to bubble up any protocol results.