Skip to content
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

fix EventReceiver does not propagate request context #540

Merged

Conversation

johejo
Copy link
Contributor

@johejo johejo commented Jun 24, 2020

Fix bug the context value set by HTTP middleware etc. was not propagated to the handler.

Signed-off-by: Mitsuo Heijo mitsuo.heijo@gmail.com

Signed-off-by: Mitsuo Heijo <mitsuo.heijo@gmail.com>
@slinkydeveloper slinkydeveloper added bug Something isn't working component/protocol/http labels Jun 24, 2020
Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits, but lgtm

})
}

handler := func(ctx context.Context) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rename this variable to eventReceiver?

if err != nil {
t.Fatal(err)
}
rh, err := client.NewHTTPReceiveHandler(context.Background(), p, handler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and rename this with httpHandler

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry I was wrong, conflicts with above.

ctx = cloudevents.ContextWithTarget(ctx, ts.URL+"/test")

result := c.Send(ctx, event)
if cloudevents.IsNACK(result) || !cloudevents.IsACK(result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can change this with

require.True(t, cloudevents.IsACK(result))

Signed-off-by: Mitsuo Heijo <mitsuo.heijo@gmail.com>
@johejo
Copy link
Contributor Author

johejo commented Jun 24, 2020

Thanks for your reviewing, has fixed.

@slinkydeveloper slinkydeveloper merged commit 8363fe8 into cloudevents:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/protocol/http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants