Empty body stops ReplyTo. by adamharwayne · Pull Request #467 · knative/eventing

adamharwayne · 2018-09-26T22:12:10Z

Proposed Changes

A ReplyTo request is not made if the body of the destination response is emtpy.

Release Note

NONE

adamharwayne · 2018-09-26T22:28:50Z

/retest

adamharwayne · 2018-09-26T22:43:32Z

/assign @markfisher because I am slightly altering the behavior of ReplyTo.

adamharwayne · 2018-09-27T15:11:22Z

/assign @markfisher

n3wscott · 2018-09-27T17:27:36Z


+// httpDoer is an interface for making HTTP requests.
+type httpDoer interface {
+	Do(*http.Request) (*http.Response, error)


I think two things:

1- if you want to put an interface infront of the http client, you need to implement all the methods:

type Client func (c *Client) Do(req *Request) (*Response, error) func (c *Client) Get(url string) (resp *Response, err error) func (c *Client) Head(url string) (resp *Response, err error) func (c *Client) Post(url, contentType string, body io.Reader) (resp *Response, err error) func (c *Client) PostForm(url string, data url.Values) (resp *Response, err error)

2 - the documentation says this: Generally Get, Post, or PostForm will be used instead of Do.

Which implies we are using it wrong and we should take a look at that.

My understanding of Go-style is that we should be creating minimal interfaces. Why add those other methods if we don't use them? If we start using them in the future, then we can add the methods to the interface.

Post() doesn't allow control of the headers. So we use Do() instead.

adamharwayne · 2018-09-27T18:36:13Z

/retest

vaikas · 2018-09-29T12:21:39Z

-		ReplyTo:   subscription.Spec.ReplyTo,
 	}
-	return b.dispatcher.DispatchMessage(message, subscriber, defaults)
+	return b.dispatcher.DispatchMessage(message, subscriber, subscription.Spec.ReplyTo, defaults)


So, we're in a bit of a wonky world right now since subscription should probably be using the one from eventing since that uses the new "approved to move forward" model. There it's Result?

+1 yeah I wonder if we need to make a copy of bus and have it use the new world subscriptions.

If we end up using the bus abstraction in the new model, then we probably should fork it. This PR is slightly modifying the DispatchMessage() interface, not really modifying anything in the bus abstraction, so I don't think it warrants copying yet.

I tend to agree with @adamharwayne that we should change the existing bus code as little as possible. We want the old objects to continue working while we build the new ones.

n3wscott · 2018-10-01T16:06:46Z

/hold
We might need to stop modifying bus and make a copy that acts on new object model.

grantr · 2018-10-01T16:32:56Z

/cc @grantr

adamharwayne · 2018-10-01T16:44:50Z

/hold cancel

This PR only modifies bus as a side effect of refactoring the DispatchMessage method signature. No functionality is being changed in bus itself.

grantr

🎉 yay for increased coverage! Thanks @adamharwayne.

/approve
/lgtm
/hold
Holding for typo fix. Feel free to cancel.

grantr · 2018-10-01T18:00:45Z

-	reply, err := d.executeRequest(destinationURL, message)
-	if err != nil {
-		return fmt.Errorf("Unable to complete request %v", err)
+func (d *MessageDispatcher) DispatchMessage(message *Message, destination, replyTo string, defaults DispatchDefaults) error {


Talked with Adam offline about the signature change that moved defaults.ReplyTo into the positional parameter replyTo. He argued that replyTo is not actually a default since it's specific to each Subscription and is never overridden.

grantr · 2018-10-01T18:27:37Z

-		ReplyTo:   subscription.Spec.ReplyTo,
 	}
-	return b.dispatcher.DispatchMessage(message, subscriber, defaults)
+	return b.dispatcher.DispatchMessage(message, subscriber, subscription.Spec.ReplyTo, defaults)


I tend to agree with @adamharwayne that we should change the existing bus code as little as possible. We want the old objects to continue working while we build the new ones.

knative-prow-robot · 2018-10-01T18:28:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adamharwayne, grantr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [grantr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

evankanderson

/lgtm

Minor nits.

knative-metrics-robot · 2018-10-01T18:57:43Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/buses/message_dispatcher.go	0.0%	89.1%	89.1

adamharwayne · 2018-10-01T19:51:21Z

/hold cancel
Comments addressed.

evankanderson · 2018-10-01T20:54:54Z

/lgtm

* Unit tests for passthrough headers. * Unit tests. * Respond to PR comments. * Remove httpDoer and use an in-memory server instead. * Respond to PR comments. * Close the response body. * Respond to PR comments. * Nil response is an error. * Unit test improvements suggested by PR comments.

Harwayne added 2 commits September 26, 2018 13:57

Unit tests for passthrough headers.

e47bd0a

Unit tests.

87dda5d

knative-prow-robot requested review from evankanderson and scothis September 26, 2018 22:12

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 26, 2018

knative-prow-robot assigned markfisher Sep 27, 2018

n3wscott reviewed Sep 27, 2018

View reviewed changes

Respond to PR comments.

1c4ee9a

Harwayne added 2 commits September 27, 2018 16:24

Remove httpDoer and use an in-memory server instead.

ab5c51d

Merge branch 'master' into buses

1ffe1bf

vaikas reviewed Sep 29, 2018

View reviewed changes

adamharwayne mentioned this pull request Oct 1, 2018

Create a Sidecar for fanning out HTTP requests #435

Merged

Harwayne added 2 commits October 1, 2018 08:44

Respond to PR comments.

fdd6842

Close the response body.

1e3c7cf

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2018

knative-prow-robot requested a review from grantr October 1, 2018 16:32

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2018

Merge branch 'master' into buses

ede214f

grantr approved these changes Oct 1, 2018

View reviewed changes

knative-prow-robot assigned grantr Oct 1, 2018

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 1, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2018

Respond to PR comments.

8c45581

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2018

evankanderson approved these changes Oct 1, 2018

View reviewed changes

Comment thread pkg/buses/message_dispatcher.go Outdated

Comment thread pkg/buses/message_dispatcher.go Outdated

Comment thread pkg/buses/message_dispatcher_test.go Outdated

knative-prow-robot assigned evankanderson Oct 1, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2018

Nil response is an error.

286dc72

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2018

Unit test improvements suggested by PR comments.

757f14b

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2018

knative-prow-robot merged commit 644409e into knative:master Oct 1, 2018

adamharwayne deleted the buses branch October 1, 2018 21:16

n3wscott mentioned this pull request Oct 24, 2018

adding n3wscott to approvers. #465

Merged

Conversation

adamharwayne commented Sep 26, 2018

Proposed Changes

Uh oh!

adamharwayne commented Sep 26, 2018

Uh oh!

adamharwayne commented Sep 26, 2018

Uh oh!

adamharwayne commented Sep 27, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamharwayne commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Oct 1, 2018

Uh oh!

grantr commented Oct 1, 2018

Uh oh!

adamharwayne commented Oct 1, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Oct 1, 2018

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Oct 1, 2018

Uh oh!

adamharwayne commented Oct 1, 2018

Uh oh!

evankanderson commented Oct 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants