New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use `curl` for the GitHub API #295

Merged
merged 9 commits into from Jun 3, 2016

Conversation

Projects
None yet
6 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented May 28, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

As discussed in #264 as curl has vastly superior proxy support. I've tried to match the code as much as possible. Split into multiple commits to make review easier.

flags = HOMEBREW_CURL_ARGS
flags.gsub!(" --progress-bar", "") if ARGV.verbose?

args = [curl, *flags.split, HOMEBREW_USER_AGENT_CURL, *args]

This comment has been minimized.

@UniqMartin

UniqMartin May 28, 2016

Contributor

This will need to be something like [curl] + flags.split + [HOMEBREW_USER_AGENT_CURL] + args to make Ruby 1.8.7 happy.

@@ -30,7 +30,7 @@
HOMEBREW_USER_AGENT_CURL = ENV["HOMEBREW_USER_AGENT_CURL"]
HOMEBREW_USER_AGENT_RUBY = "#{ENV["HOMEBREW_USER_AGENT"]} ruby/#{RUBY_VERSION}-p#{RUBY_PATCHLEVEL}"

HOMEBREW_CURL_ARGS = "-f#RLA"
HOMEBREW_CURL_ARGS = "--fail --progress-bar --remote-time --location --user-agent"

This comment has been minimized.

@UniqMartin

UniqMartin May 28, 2016

Contributor

What do you think about making this an array instead? This will also make proper argument assembly where this is used easier, and filtering individual curl arguments won't need to rely on String#sub and could instead use Array#- (e.g. HOMEBREW_CURL_ARGS - %w[--progress-bar]). It seems like adding the user agent at this stage would make sense, too. (But I'm less sure about this second part.) To be more specific:

HOMEBREW_CURL_ARGS = %W[
  --fail
  --progress-bar
  --remote-time
  --location
  --user-agent #{HOMEBREW_USER_AGENT_CURL}
]

(Code using this will of course need to be adjusted slightly, but mostly just can omit the .split.)

@@ -30,7 +30,7 @@
HOMEBREW_USER_AGENT_CURL = ENV["HOMEBREW_USER_AGENT_CURL"]
HOMEBREW_USER_AGENT_RUBY = "#{ENV["HOMEBREW_USER_AGENT"]} ruby/#{RUBY_VERSION}-p#{RUBY_PATCHLEVEL}"

This comment has been minimized.

@UniqMartin

UniqMartin May 28, 2016

Contributor

It seems that HOMEBREW_USER_AGENT_RUBY is no longer used anywhere in the code.

This comment has been minimized.

@apjanke

apjanke May 28, 2016

Contributor

Perhaps it should be, in brew pull. My new code isn't setting the user agent anywhere, I think.

This comment has been minimized.

@UniqMartin

UniqMartin May 28, 2016

Contributor

You mean the “Waiting on Bintray” code? Yes, I think it should be using this, but nobody noticed the omission thus far.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 29, 2016

Member

Added in b0262f3

@@ -0,0 +1,24 @@
require "pathname"

def curl_args(*args)

This comment has been minimized.

@UniqMartin

UniqMartin May 28, 2016

Contributor

curl_args seems to always be called with a preassembled array. Maybe you want to get rid of the “collect” (*) here and omit the splat (*) at the two call sites I've seen?

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented May 28, 2016

A few small suggestions, but nothing major. Already very solid on the first iteration as far as I can tell. 👍 (And splitting the commits was indeed very helpful for review.)

require "net/https"

headers = api_headers
tmpfile = Tempfile.new("github_api_headers", HOMEBREW_TEMP)

This comment has been minimized.

@UniqMartin

UniqMartin May 28, 2016

Contributor

Maybe this is overkill, but the Tempfile documentation suggests to explicitly clean up the temporary file by doing something like this:

tmpfile = Tempfile.new("github_api_headers", HOMEBREW_TEMP)
begin
  # Use `tmpfile`.
ensure
  tmpfile.close
  tmpfile.unlink
end

(It should be fine as-is, too, as it is being removed once the Tempfile instance is garbage collected.)

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 29, 2016

@MikeMcQuaid just want to ask what's your thought on #264 (comment)

I will do some test to confirm my theory. But it looks like that we could keep using Ruby HTTP API by using net/http instead of open-uri like what we did in gist-logs.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 29, 2016

If we switch to curl, it may be worthy to make gist-logs to do the same.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2016

@MikeMcQuaid just want to ask what's your thought on #264 (comment)

I think it's fine for Homebrew developer-centric stuff to not use curl. That said, I'm pretty sure that it's possible somehow using curl.

If we switch to curl, it may be worthy to make gist-logs to do the same.

Yep, I'll address that as part of this PR. The goal is to having anything that end-users use to use curl; even if the Ruby HTTP API works well it'll still handle proxies/authentication slightly differently so I think it's worth using curl universally.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 29, 2016

If we choose to use curl, here is a list of issues need to be addressed:

  • Compatible on old system. Will old system cert problem impact this?
  • Security.
    • If I run brew --verbose, the token should not be displayed.
    • If possible, can we not create a new temp file? It may leak sensitive information on multi users machine. Can we use file descriptor and StringIO instead?
  • Performance, a benchmark is preferable compared with Ruby API.
  • Robust, we could use mitproxy to test different proxies to make sure new implementation do solve the problem. And since we are implementing lots of lower level details on our own, test case may be required.
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2016

Compatible on old system. Will old system cert problem impact this?

We already use curl to download lots of things from GitHub already.

If possible, can we not create a new temp file? It may leak sensitive information on multi users machine. Can we use file descriptor and StringIO instead?

What sensitive information?

Performance, a benchmark is preferable compared with Ruby API.

This code is not performance critical and better proxy support and consistency trumps performance.

Robust, we could use mitproxy to test different proxies to make sure new implementation do solve the problem. And since we are implementing lots of lower level details on our own, test case may be required.

Unneeded, we already rely on curl for most HTTP transfers already.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 29, 2016

We already use curl to download lots of things from GitHub already.

But we are using --insecure on old system at least for tigerbrew, which may be fine for fetching but not for sending request with token.

What sensitive information?

cookies and token.

This code is not performance critical and better proxy support and consistency trumps performance.

I mean better performance is always preferable. Also I don't believe Ruby has less proxy support(except open-uri)

Unneeded, we already rely on curl for most HTTP transfers already.

We never use curl do auth transfers over auth proxy. And by test case, I mean to test the code parse the headers/http code/output

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2016

cookies and token.

We are not sending cookies and tokens are not included in the returned headers.

Also I don't believe Ruby has less proxy support(except open-uri)

If you would like to remove open-uri support and instead use net/http in another pull request: please feel free. I will not be doing it in this one.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 29, 2016

If you would like to remove open-uri support and instead use net/http in another pull request: please feel free. I will not be doing it in this one.

I can sure submit a PR for comparison. Just want to compare two approaches more thoroughly. 😉

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2016

@xu-cheng 🆒, I just don't think that comparison needs to necessarily block this PR being merged and I think there's merit in standardising on a single method of HTTP communication.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:github-api-curl branch from 0111414 to b0262f3 May 29, 2016

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented May 29, 2016

I think there's merit in standardising on a single method of HTTP communication.

Seconded. It also makes us more consistent with respect to HTTPS handling, e.g. because we're not consulting possibly different certificate stores. I'm also convinced that network speed is the dominating factor here and not how quickly we can shell out to curl vs. some native API. And it's not like we're sending hundreds of requests in short succession, we're only sending a few every once in a while.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2016

I'm also convinced that network speed is the dominating factor here and not how quickly we can shell out to curl vs. some native API.

👍

--remote-time
--location
--user-agent #{HOMEBREW_USER_AGENT_CURL}
]

This comment has been minimized.

@UniqMartin

UniqMartin May 29, 2016

Contributor

One small nit here and something I forgot in my previous suggestion: Maybe append a .freeze? It would both slightly benefit performance and—more importantly—prevent us from accidentally modifying this constant. (I believe RuboCop would have suggested this, too.)

end
end

def open(url, &_block)

This comment has been minimized.

@UniqMartin

UniqMartin May 29, 2016

Contributor

I believe the &_block argument can be dropped from the argument list since this method never needs to pass the block as a regular argument and only ever uses yield.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 29, 2016

And it's not like we're sending hundreds of requests in short succession, we're only sending a few every once in a while.

How about brew search?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2016

@xu-cheng We're not sending hundreds in that case, either 😉

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:github-api-curl branch from b0262f3 to 7c5a608 May 29, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2016

Pushed and addressed feedback. This should be ready for testing.

end
def create_gist(files, description)
data = { "public" => true, "files" => files, "description" => description }
GitHub.open("https://api.github.com/gists", data)["html_url"]
end

This comment has been minimized.

@xu-cheng

xu-cheng May 30, 2016

Contributor

A style nit, these functions may be more suitable to add into Github module from a semantics aspect.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 30, 2016

Member

Yeh, perhaps. It feels a little weird if they are only used in this file, though, so I'd rather hold off for now.

puts
request.basic_auth(user, password)
end

This comment has been minimized.

@xu-cheng

xu-cheng May 30, 2016

Contributor

Any chance to drop these environment by put login! into Github module, so users won't set this env by their own.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 30, 2016

Member

Good idea.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 31, 2016

Any objections to 🚢ing the current code and iterating on any issues? Happy to wait for folks here to do more review or testing but just want to be sure I'm waiting on the right people 😀

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jun 1, 2016

I'm 👍 on shipping as-is. Did work as expected in my (admittedly rather limited) testing.

@MikeMcQuaid MikeMcQuaid merged commit 8e0e164 into Homebrew:master Jun 3, 2016

1 check passed

default Build finished.
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:github-api-curl branch Jun 3, 2016

@BrewTestBot BrewTestBot removed the in progress label Jun 3, 2016

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jun 5, 2016

@MikeMcQuaid Did you intentionally squash merge this PR? I think it would have been useful to preserve the individual commits in our history.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 5, 2016

@UniqMartin Will bear that in mind in future. Do we want to enable merge commits on this repository now for this situation, do you think?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 5, 2016

(you can enable both)

end
def new_issue(repo, title, body)
data = { "title" => title, "body" => body }
GitHub.open("https://api.github.com/repos/MikeMcQuaid/test/issues", data)["html_url"]

This comment has been minimized.

@tkelman

tkelman Jun 5, 2016

Contributor

was this test url left in intentionally?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 5, 2016

Member

It was definitely not, thanks.

This comment has been minimized.

@MikeMcQuaid
@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jun 5, 2016

I'm personally fond of the linear history and thus tend to use brew pull for multi-commit PRs, but I can totally see the appeal for being able to merge directly from the GitHub website without having to resort to the command line.

Do we want to enable merge commits on this repository now for this situation, do you think?

Now that we have a separate repository for the package manager, I think this makes sense and wouldn't have a noticeably negative impact on the readability of our history. For changes like these I'd certainly prefer the context and step-by-step nature of individual commits in a side branch, that would be retained by a merge commit. (But I was really just curious whether the squash merge was a conscious decision.)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 5, 2016

Now that we have a separate repository for the package manager, I think this makes sense and wouldn't have a noticeably negative impact on the readability of our history.

🆒 done 👍

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jun 5, 2016

🆒 done 👍

Thanks! 🙇

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.