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

New feature: Allow download from private GitHub repository #1763

Merged
merged 6 commits into from Jan 16, 2017

Conversation

Projects
None yet
4 participants
@minamijoyo
Copy link
Contributor

minamijoyo commented Jan 2, 2017

Why?

I would like to download the release asset of the private GitHub repository.

I'm using a private brew tap at my company and the prebuild binaries for internal tools are uploaded to the private GitHub Release page.
For such in-house use, S3DownloadStrategy already exists, but most of the source code are managed by GitHub, I think it would be useful to download the binary directly from GitHub.

What I did

I implemented a new DownloadStrategy to find and download asset using GitHub API.

How I did it

First, resolve the asset_id associated with the release tag with the following API:
https://developer.github.com/v3/repos/releases/#get-a-release-by-tag-name

and then, download the asset with the following API:
https://developer.github.com/v3/repos/releases/#get-a-single-release-asset

How to verify it

To use it, add :using => GitHubReleaseDownloadStrategy to the URL section of your formula.

require "formula"

class Hoge < Formula
  url "https://github.com/:owner/:repo/releases/download/:tag/hoge_v0.1.0_darwin_amd64.tar.gz", :using => GitHubReleaseDownloadStrategy
  ...

end
$ export HOMEBREW_GITHUB_API_TOKEN=xxxxxx
$ brew install hoge

Please review this.

Check List

  • 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?
New feature: GitHubReleaseDownloadStrategy
GitHubReleaseDownloadStrategy downloads tarballs from GitHub Release assets.
To use it, add ":using => GitHubReleaseDownloadStrategy" to the URL section
of your formula. This download strategy uses GitHub access tokens (in the
environment variables GITHUB_TOKEN) to sign the request.
This strategy is suitable for corporate use just like S3DownloadStrategy,
because it lets you use a private GttHub repository for internal distribution.
It works with public one, but in that case simply use CurlDownloadStrategy.

@bfontaine bfontaine added the features label Jan 2, 2017

# because it lets you use a private GttHub repository for internal distribution.
# It works with public one, but in that case simply use CurlDownloadStrategy.
class GitHubReleaseDownloadStrategy < CurlDownloadStrategy
require 'open-uri'

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 2, 2017

Member

This should use utils/curl and/or utils/github instead.

This comment has been minimized.

@minamijoyo

minamijoyo Jan 3, 2017

Contributor

Oh, I did not know such a useful lib. I've changed to use utils/github.
I also have changed the environment varaible used to this same HOMEBREW_GITHUB_API_TOKEN .


@github_token = ENV["GITHUB_TOKEN"]
unless @github_token
puts "Environmental variable GITHUB_TOKEN is required."

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 2, 2017

Member

Perhaps this string should be part of the raise instead?

This comment has been minimized.

@minamijoyo

minamijoyo Jan 3, 2017

Contributor

Sure, I've changed it.


url_pattern = %r|https://github.com/(\S+)/(\S+)/releases/download/(\S+)/(\S+)|
unless @url =~ url_pattern
puts "Invalid url pattern for GitHub Release."

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 2, 2017

Member

Perhaps this string should be part of the raise instead?

This comment has been minimized.

@minamijoyo

minamijoyo Jan 3, 2017

Contributor

I've updated it here as well.

raise CurlDownloadStrategyError, @url
end

url_pattern = %r|https://github.com/(\S+)/(\S+)/releases/download/(\S+)/(\S+)|

This comment has been minimized.

@MikeMcQuaid

This comment has been minimized.

@minamijoyo

minamijoyo Jan 3, 2017

Contributor

@MikeMcQuaid I am trying to support this format, but the following URL does not contain any asset.
https://api.github.com/repos/Homebrew/brew/releases/tags/1.1.5

How do we download it? I could not find an GitHub API to get archive. Are there any good ideas?

This comment has been minimized.

@minamijoyo

minamijoyo Jan 8, 2017

Contributor

I've supported both cases.

return assets.first["id"]
end

def release_url

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 3, 2017

Member

I reckon you can just make this a variable or inline it rather than a function.

This comment has been minimized.

@minamijoyo

minamijoyo Jan 8, 2017

Contributor

Fixed.

end

def _fetch
puts "Download asset_id: #{asset_id}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 3, 2017

Member

I think this shouldn't be outputted as it's not necessary for the user to see.

This comment has been minimized.

@minamijoyo

minamijoyo Jan 8, 2017

Contributor

Fixed.

super

@github_token = ENV["HOMEBREW_GITHUB_API_TOKEN"]
raise CurlDownloadStrategyError, "Environmental variable HOMEBREW_GITHUB_API_TOKEN is required." unless @github_token

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 3, 2017

Member

Use unless ... end so this line is shorter.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 3, 2017

Member

Also, you probably need to handle the case where the token doesn't have e.g. private repository access.

This comment has been minimized.

@minamijoyo

minamijoyo Jan 8, 2017

Contributor

Fixed.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 3, 2017

@MikeMcQuaid I am trying to support this format, but the following URL does not contain any asset.
https://api.github.com/repos/Homebrew/brew/releases/tags/1.1.5

How do we download it? I could not find an GitHub API to get archive. Are there any good ideas?

@minamijoyo I'm thinking this download strategy could be a lot more general and instead be e.g. GitHubTokenDownloadStrategy. You can access private resources like if e.g. https://github.com/Homebrew/brew/archive/1.1.5.tar.gz was private using the same token format you've used but without the API call i.e. just a curl https://$TOKEN@github.com/Homebrew/brew/archive/1.1.5.tar.gz. That means we can avoid any API calls or parsing and just allow this download strategy to take a token and pass that to GitHub for downloading any private resource. Thoughts?

@minamijoyo

This comment has been minimized.

Copy link
Contributor

minamijoyo commented Jan 3, 2017

@MikeMcQuaid Thanks for review and advice.
First let me share my current understanding.

Certainly the archive url can be accessed with the curl.
https://$TOKEN@github.com/:owner/:repo/archive/:filename

But the releases url is not. In the case of the a private repository, even if you set a token, not found is returned. I don't know why there is such a difference. Try it with a private repository.
https://$TOKEN@github.com/:owner/:repo/releases/download/:tag/:filename

Although the url of assets API can be accessed with the curl, asset_id is not shown on Web, I don't know how to get asset_id without API.
https://$TOKEN@api.github.com/repos/:owner/:repo/releases/assets/:asset_id

In addition, to download the asset's binary content, we must set the Accept header of the request to application/octet-stream.

Therefore it is difficult to make it a common implementation without API calls. Is there a way to go something good? If we assume that both logic are implemented respectively, I think that it is better to separate them into different classes, but one is not what I originally wanted to do.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 7, 2017

@minamijoyo I think it's fine to have API calls if they are needed for releases 👍 but I think this PR should probably handle both cases in order to be useful. The GitHub Releases download strategy could be a subclass from a GitHub Private Repository download strategy to share e.g. at least the token logic.

But the releases url is not. In the case of the a private repository, even if you set a token, not found is returned. I don't know why there is such a difference.

It may be worth asking support@github.com why this is the case and their thoughts; they will definitely be able to help.

@minamijoyo

This comment has been minimized.

Copy link
Contributor

minamijoyo commented Jan 7, 2017

@MikeMcQuaid OK. I will try to support both cases. Which of the base class name should be GitHubTokenDownloadStrategy or GitHubPrivateRepositoryDownloadStrategy ?

Also I will also ask GitHub why there is such a difference.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 7, 2017

@minamijoyo Thanks! I think GitHubPrivateRepositoryDownloadStrategy and GitHubPrivateRepositoryReleaseDownloadStrategy are the best names.

@minamijoyo

This comment has been minimized.

Copy link
Contributor

minamijoyo commented Jan 7, 2017

@MikeMcQuaid OK. I will do so.

@minamijoyo minamijoyo changed the title New feature: GitHubReleaseDownloadStrategy New feature: Allow download from private GitHub repository Jan 8, 2017

@minamijoyo

This comment has been minimized.

Copy link
Contributor

minamijoyo commented Jan 8, 2017

@MikeMcQuaid I've fixed it, so please review again.
In addition, as the class name changed, I've also changed the title of this pull request to a more appropriate name.

@minamijoyo minamijoyo changed the title New feature: Allow download from private GitHub repository New feature: Allow downloading from private GitHub repository Jan 8, 2017

@minamijoyo minamijoyo changed the title New feature: Allow downloading from private GitHub repository New feature: Allow download from private GitHub repository Jan 8, 2017

@minamijoyo

This comment has been minimized.

Copy link
Contributor

minamijoyo commented Jan 9, 2017

I contacted GitHub's support and got answers. The summary is as follows:

  • Is it possible to download private release assets directly with curl without using API?

No, not currently.

  • If possible please tell me how to do it.

It's not possible.

  • If not, compared to the archive curl, why is there such a difference?

The fact that it's possible to do for the archive doesn't mean it will be possible for everything else -- you should not have such expectations and I don't believe we'll support that for releases anytime soon. To clarify a bit why those two are different -- those two things are stored differently and go through different paths. In general, you should be using the API for all automation purposes.

After all, we can not get the private release assets without API.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 16, 2017

Fantastic work @minamijoyo. Thanks for your first contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@joscha @stefansundin This should help you with your use-cases.

@MikeMcQuaid MikeMcQuaid merged commit 133e597 into Homebrew:master Jan 16, 2017

3 checks passed

codecov/patch 85.10% of diff hit (target 63.03%)
Details
codecov/project 63.11% (+0.08%) compared to 3e0d54d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joscha

This comment has been minimized.

Copy link

joscha commented Jan 16, 2017

Nice work @minamijoyo!

@minamijoyo

This comment has been minimized.

Copy link
Contributor

minamijoyo commented Jan 17, 2017

@MikeMcQuaid Thank you for review 🍺

@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.