gh: Move away from using curl for patches and to GitHub API #601
Conversation
|
I could help with this since it involves changes to octokit. I could start as early as this evening. |
|
Thanks! |
|
i just rebased this branch on top of the latest |
|
@jingweno Hey do you need any help finalizing the patches API so we can download them from hub? |
|
@mislav Yes, please help a bit. I've added the commit patch API. The pull request patch API should be similar. The gist api is half way there since I haven't added the method to return raw. I'll try to come back to this project this week. |
|
@jingweno Now that go-octokit is ready for this, do you want me to try fixing this today/tomorrow or are you already on this? |
|
@mislav Please go ahead. I'll move on to disabling autoupdate for homebrew build |
mislav
force-pushed
the
no-curl
branch
2 times, most recently
from
99020f4
to
740a5d3
Compare
In testing, we want to redirect all requests to a local test server pretending to be GitHub API, but we want them to preserve their original "Host" header. A good place to do that transparently is the `RoundTrip` method that handles every request for this http.Client instance.
This enables fetching patches from private repos that you have access to.
|
@jingweno Ready for 👀, thanks |
| if gist { | ||
| prefix = "gist-" | ||
| } | ||
| bytes, err := ioutil.ReadAll(patch) |
There was a problem hiding this comment.
io.Copy is a better way do copy from a reader to writer:
diff --git a/commands/apply.go b/commands/apply.go
index 8521886..8b5d06e 100644
--- a/commands/apply.go
+++ b/commands/apply.go
@@ -3,7 +3,6 @@ package commands
import (
"io"
"io/ioutil"
- "os"
"regexp"
"github.com/github/hub/github"
@@ -94,13 +93,12 @@ func transformApplyArgs(args *Args) {
}
idx := args.IndexOfParam(arg)
- patchFile, err := ioutil.TempFile(os.TempDir(), "hub")
+ patchFile, err := ioutil.TempFile("", "hub")
utils.Check(err)
- bytes, err := ioutil.ReadAll(patch)
+ _, err = io.Copy(patchFile, patch)
utils.Check(err)
- patchFile.Write(bytes)
patchFile.Close()
patch.Close()Also ioutil.TempFile uses os.TempDir() by default already.
|
Looking 👍 |
| }) | ||
|
|
||
| c := newHttpClient(s.URL.String(), false) | ||
| c.Get(fmt.Sprintf("%s/test", s.URL)) |
There was a problem hiding this comment.
The whole point of OverrideURL is that it rewrites Host and Scheme. This test doesn't verify that, as it makes a request to the same host:port that the test server is running at.
Instead, try to make a request to "https://example.com" and verify that the X-Original-Scheme is "https" and that the request host was "example.com".
There was a problem hiding this comment.
Also, we need a test that verifies a very important condition: that the http client won't rewrite URLs if OverrideURL is nil. So, we should initiate newHttpClient("", false), make a request to the test server, and verify that the hostname (probably just "127.0.0.1") is unchanged and that X-Original-Scheme was not set.
There was a problem hiding this comment.
Fixed in 5603206 (--amend)
owenthereal
force-pushed
the
no-curl
branch
from
9723693
to
7020d62
Compare
owenthereal
force-pushed
the
no-curl
branch
from
7020d62
to
5603206
Compare
gh: Move away from using curl for patches and to GitHub API
|
@jingweno Thanks! Shipped. |
am/applycommands. We need to switch them over to the GitHub API so that they won't have any external dependencies./cc @jingweno