octokit / go-octokit

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

Automatically respect $http_proxy & $no_proxy #53

Closed mislav closed 10 years ago

mislav commented 10 years ago

Faraday does it by default. Makes debugging in tests way easier. I don't know whether this belongs in Octokit or Sawyer; @technoweenie?

From net/http docs:

tr := &http.Transport{
    Proxy: # ???
}
client := &http.Client{Transport: tr}
resp, err := client.Get("https://example.com")
  1. http://golang.org/pkg/net/http/#ProxyFromEnvironment
  2. http://golang.org/pkg/net/http/#Transport
owenthereal commented 10 years ago

There's a method to pass in http.Client in which you could build your own transport. In test, it uses the default client (if it's nil, it uses the default one).

mislav commented 10 years ago

Thanks, that makes sense. However, I tried doing this in gh:

func (client *Client) octokit() (c *octokit.Client) {
    tokenAuth := octokit.TokenAuth{AccessToken: client.Credentials.AccessToken}
    u, _ := url.Parse("http://localhost:8888")
    tr := &http.Transport{Proxy: http.ProxyURL(u)}
    httpClient := &http.Client{Transport: tr}
    c = octokit.NewClientWith(client.apiEndpoint(), httpClient, tokenAuth)

    return
}

But when I run the fork command, it performs HTTP requests as before, but they don't end up in my debugging proxy at port 8888. Not sure what I'm doing wrong.

mislav commented 10 years ago

Oh, I'm sorry. It actually worked, but due to some filtering issues in my debugging proxy they weren't getting displayed. My bad :blush:

Thanks!

mislav commented 10 years ago

Here's what I found in the meantime digging through net/http source:

The default transport for all HTTP requests actually should respect $HTTP_PROXY by default:

var DefaultTransport RoundTripper = &Transport{Proxy: ProxyFromEnvironment}

I tested it with go programs and it's true—unless the original request is to 127.0.0.1, when go decides that you don't want a proxy since the request is to localhost. :angry: This is bullshit, since this is precisely my use case: I want to debug requests made to a local test server.

So now I have to write my own version of ProxyFromEnvironment that doesn't special case 127.0.0.1.

owenthereal commented 10 years ago

:+1: Good find! Somehow missed this issue.