Skip to content

fix EventReceiver does not propagate request context#540

Merged
slinkydeveloper merged 2 commits intocloudevents:masterfrom
johejo:fix_http_handler_with_context
Jun 24, 2020
Merged

fix EventReceiver does not propagate request context#540
slinkydeveloper merged 2 commits intocloudevents:masterfrom
johejo:fix_http_handler_with_context

Conversation

@johejo
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread v2/client/http_receiver_test.go Outdated
})
}

handler := func(ctx context.Context) error {
Copy link
Copy Markdown
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?

Comment thread v2/client/http_receiver_test.go Outdated
if err != nil {
t.Fatal(err)
}
rh, err := client.NewHTTPReceiveHandler(context.Background(), p, handler)
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread v2/client/http_receiver_test.go Outdated
ctx = cloudevents.ContextWithTarget(ctx, ts.URL+"/test")

result := c.Send(ctx, event)
if cloudevents.IsNACK(result) || !cloudevents.IsACK(result) {
Copy link
Copy Markdown
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
Copy Markdown
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