octokit / go-octokit

Simple Go wrapper for the GitHub API
https://github.com/octokit/go-octokit
MIT License
258 stars 79 forks source link

Apply global headers #60

Closed owenthereal closed 10 years ago

owenthereal commented 10 years ago

The changes allow us to have callbacks before sending requests and after receiving response (before the JSON deserialization). Previously the request/response cycle is all hidden and this'll open up an extension point. A use case in the new Hub:

type HubTestMiddleware struct {}
func (m *HubTestMiddleware) PrepareRequest(req *octokit.Request) error {
  req.Header.Set("X-HUB-HOST", "api.github.com")
}
func (m *HubTestMiddleware) PrepareResponse(resp *octokit.Response) error {
  return nil
}

...

client.Use(&HubTestMiddleware{})

And the cuke would be able to receive some custom headers.

technoweenie commented 10 years ago

I prefer something like the RoundTripper interface to a middleware system.

What kind of stuff do you imagine setting in this in Octokit? I figure Octokit should support for various custom headers that the GitHub API needs.

owenthereal commented 10 years ago

The intention is to being able to set extra headers so that the cukes of the new Hub are happy. For example, I need to set a HOST_NAME header to make the test pass.

RoundTripper looks pretty cool. But the doc says:

RoundTrip should not modify the request, except for consuming and closing the Body. The request's URL and Header fields are guaranteed to be initialized.

I wonder if there's any problem if I mutate the headers. I'll do more research on that though.

Good call on adding more support on the custom headers that GitHub API requires in go-octokit. Currently it only sets the mandatory headers. For those are optional, e.g. If-Modified-Since, we don't provide options for that. So the outcome of this PR will be able to fix it.

owenthereal commented 10 years ago

@technoweenie I just read the source of how RoundTripper is used. I didn't see any side effect of mutating the headers. But I'll ask in the Google group to confirm.

To use RoundTripper as the middleware system, are you looking to use it like this? http://play.golang.org/p/pem5CZKInL

technoweenie commented 10 years ago

I don't think RoundTripper is really intended to modify a header like that. You typically have the actual request object while using the HTTP client, so you can just add your headers there.

RoundTripper might be better if you want a specific header on ALL requests. But go-sawyer should give you this capability already.

client := sawyer.NewFromString("https://api.github.com")

// header set on EVERY request
client.Headers.Set("Accept", "application/vnd.github+json")

apierr := &ApiError{} // decoded from response body on non-20x responses
user := &User{}
req := client.NewRequest("user/21", apierr)

// header set on just this request
req.Headers.Set("X-Foo", "Bar")
owenthereal commented 10 years ago

@technoweenie What do you think of 25a2655? I removed the middleware system and prefer Client.Header instead. Unfortunately we don't expose the Request object since go-octokit was designed to be a higher level abstraction. There's one caveat to pay attention to though as we discussed in https://github.com/github/hub/pull/535#discussion_r11182262. Details see https://code.google.com/p/go/issues/detail?id=7682.

technoweenie commented 10 years ago

:+1:

owenthereal commented 10 years ago

Wohoo, merging