Add `hub release download` command that downloads all assets. #1103

glennpratt · 2016-02-05T23:26:36Z

This is probably overly simplistic, it downloads all assets if there are any, writes them in the current directory and prints the names.

I was lazy and avoided touching octokit.

Addresses #1102

glennpratt · 2016-02-06T05:10:22Z

I'm thinking for completeness this should have some args:

--all
--source-zip
--source-tar
--assets

Not sure what the default should be (or perhaps there shouldn't be one). A filter for --assets might be nice, but I don't need one 😎

mislav · 2016-02-09T01:47:15Z

github/client.go

+		return
+	}
+
+	asset, err = api.GetBody(url, "application/octet-stream")


Could we avoid adding a new GetBody function and simply use api.Get to obtain a response object and return its .Body property from this function? I think it will result in less overall added code.

@mislav Get() doesn't appear to allow setting the Accept header. Should I change it's signature?

@mislav I just assumed the answer was yes. See new diff.

Get() doesn't appear to allow setting the Accept header. Should I change it's signature?

Oh I see. I don't think we should be changing Get signature across the board. I forgot that Get doesn't support a configure block. I would go back to your initial idea of creating a specialized method.

mislav · 2016-02-09T07:53:24Z

Thanks for your pull request! This is a good start, but we'll have to polish this command's outside API a bit. First of all, what would be the use-case for this command? Will people usually want to download all assets, or just a specific one? Will the GET http request need to follow redirects? Etc.

Thanks for your patience!

glennpratt · 2016-02-09T15:07:51Z

@mislav

Use case: I'm using it to download official releases of our private software (compiled), so that we aren't compiling it over and over and have consistent builds. In our case, everyone has a checkout available already, but it could be nice to even support a project path arg (:user/:repo) and skip the checkout.
Asset filter: I'm downloading all because I only have one asset per release (only have one target). That would probably be different for others - but should that be a blocker or a future feature? I definitely do not want the source assets and am not sure many would vs Git, but that's easy enough to support.
Redirects: according to the docs, the GET request must follow redirects and presumably does.

mislav · 2016-02-09T21:46:48Z

but it could be nice to even support a project path arg (:user/:repo) and skip the checkout.

I had this idea that supporting a :user/:repo arg across the board to specify a non-checked out repo would be a great addition to hub.

but should that be a blocker or a future feature?

Not necessarily a blocker for this PR, but I would definitely like to reach a decision before this subcommand hits the next feature release.

according to the docs, the GET request must follow redirects and presumably does.

I don't think it does… the release family of commands uses a rudimentary API client called simpleApi internally that has a very lightweight implementation and no redirect following. I think you would have to implement that for DownloadAsset.

glennpratt · 2016-02-09T23:48:45Z

@mislav it is following redirects. The final URL is available in the response:

response.Request.URL.String()

A test, just adding a couple prints to the current code (sensitive parts changed):

url
https://api.github.com/repos/myorg/my-project/releases/assets/1292217

response.Request.URL.String()
https://github-cloud.s3.amazonaws.com/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAI000000000000000%FFFFFFFFF%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20160209T233542Z&X-Amz-Expires=300&X-Amz-Signature=0000000000000000000000000000000000000000000000000000&X-Amz-SignedHeaders=host&actor_id=0&response-content-disposition=attachment%3B%20filename%3Dmy-project-0.0.0-gpratt7-linux-3.19.0-30-generic-x86_64.tar.gz&response-content-type=application%2Foctet-stream

mislav · 2016-02-10T03:03:59Z

github/client.go

+		return
+	}
+
+	result, err := api.Get(url, func(req *http.Request) {


Could this use performRequest so we don't have to change Get() signature?

Not directly because performRequest isn't exported, but I added a GetFile().

mislav · 2016-02-10T03:05:04Z

Oh wow, I never realized this API client would follow redirects automatically. I'm not sure where it happens, but it's awesome if it works.

Could you add a cuke for this feature to features/release.feature?

mislav · 2016-02-15T03:17:25Z

commands/release.go

+
+		ui.Printf("Downloading %s ...\n", asset.Name)
+
+		assetFile, err := os.OpenFile(asset.Name, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600)


How about os.CreateFile?

If you mean os.Create, it truncates an existing file. I fail if there is an existing file (O_EXCL). Ideally I would append (n) if the file already exists like a browser might do.

Gotcha, makes sense. How about using 0644 as default permissions?

mislav · 2016-02-15T03:21:03Z

Few nits, but otherwise this is coming together really nicely! 👍 thanks for your patience

glennpratt · 2016-02-15T03:36:06Z

Thanks for the comments. I'm poking at the cucumber test as I get free time - though I don't have much the next few days.

glennpratt · 2016-02-16T03:17:46Z

Added cucumber scenario. I'll address your comments as a get more free time.

glennpratt · 2016-02-16T04:37:42Z

@mislav addressed your comments.

mislav · 2016-02-16T06:28:13Z

features/release.feature

+          headers['Location'] = 'https://github-cloud.s3.amazonaws.com/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz'
+          ""
+        }
+        get('/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz') {


Could you append the :host_name => 'github-cloud.s3.amazonaws.com' constraint to this route so it serves as an assertion that this route was called with the correct host name and not api.github.com? Thanks

@mislav good idea, just tried.

I get a 404. If I return the host header in the current method, it's 127.0.0.1. I'm not sure what's happening here but I suspect it's an artifact of the testing framework.

get('/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz') { headers['Content-Type'] = 'application/octet-stream' request.host }

expected: "ASSET_TARBALL" got: "127.0.0.1"

Strange. That should work, since we're using :host_name constraint elsewhere in the test suite. When debugging, I think you should probably look at request.headers['host'] or request.env['HTTP_HOST'] since request.host is not guaranteed to be accurate.

Hrmm, maybe the Go client isn't redirecting correctly. I'll try changing the code to match the redirect tooling I see in Get() though it might be a few days before I do that.

tcpflow output:

127.000.000.001.53925-127.000.000.001.53924: GET /__identify__ HTTP/1.1 Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3 Accept: */* User-Agent: Ruby Host: 127.0.0.1:53924 127.000.000.001.53925-127.000.000.001.53924: GET /__identify__ HTTP/1.1 Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3 Accept: */* User-Agent: Ruby Host: 127.0.0.1:53924 127.000.000.001.53924-127.000.000.001.53925: HTTP/1.1 200 OK Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Content-Length: 14 Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53925: HTTP/1.1 200 OK Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Content-Length: 14 Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53925: 70296040667240 127.000.000.001.53924-127.000.000.001.53925: 70296040667240 127.000.000.001.53928-127.000.000.001.53924: GET /repos/mislav/will_paginate/releases HTTP/1.1 Host: api.github.com User-Agent: Go-http-client/1.1 Authorization: token OTOKEN X-Original-Port: 80 X-Original-Scheme: https Accept-Encoding: gzip 127.000.000.001.53928-127.000.000.001.53924: GET /repos/mislav/will_paginate/releases HTTP/1.1 Host: api.github.com User-Agent: Go-http-client/1.1 Authorization: token OTOKEN X-Original-Port: 80 X-Original-Scheme: https Accept-Encoding: gzip 127.000.000.001.53924-127.000.000.001.53928: HTTP/1.1 200 OK Content-Type: application/json;charset=utf-8 Content-Length: 332 Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53928: HTTP/1.1 200 OK Content-Type: application/json;charset=utf-8 Content-Length: 332 Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53928: [{"url":"https://api.github.com/repos/mislav/will_paginate/releases/123","upload_url":"https://api.github.com/uploads/assets{?name,label}","tag_name":"v1.2.0","name":"will_paginate 1.2.0","draft":true,"prerelease":false,"assets":[{"url":"https://api.github.com/repos/mislav/will_paginate/assets/9876","name":"hello-1.2.0.tar.gz"}]}] 127.000.000.001.53924-127.000.000.001.53928: [{"url":"https://api.github.com/repos/mislav/will_paginate/releases/123","upload_url":"https://api.github.com/uploads/assets{?name,label}","tag_name":"v1.2.0","name":"will_paginate 1.2.0","draft":true,"prerelease":false,"assets":[{"url":"https://api.github.com/repos/mislav/will_paginate/assets/9876","name":"hello-1.2.0.tar.gz"}]}] 127.000.000.001.53929-127.000.000.001.53924: GET /repos/mislav/will_paginate/assets/9876 HTTP/1.1 Host: api.github.com User-Agent: Go-http-client/1.1 Authorization: token OTOKEN Content-Type: application/octet-stream X-Original-Port: 80 X-Original-Scheme: https Accept-Encoding: gzip 127.000.000.001.53929-127.000.000.001.53924: GET /repos/mislav/will_paginate/assets/9876 HTTP/1.1 Host: api.github.com User-Agent: Go-http-client/1.1 Authorization: token OTOKEN Content-Type: application/octet-stream X-Original-Port: 80 X-Original-Scheme: https Accept-Encoding: gzip 127.000.000.001.53924-127.000.000.001.53929: HTTP/1.1 302 Found Content-Type: application/json;charset=utf-8 Location: https://github-cloud.s3.amazonaws.com/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz Content-Length: 2 Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53929: HTTP/1.1 302 Found Content-Type: application/json;charset=utf-8 Location: https://github-cloud.s3.amazonaws.com/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz Content-Length: 2 Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53929: {} 127.000.000.001.53924-127.000.000.001.53929: {} 127.000.000.001.53929-127.000.000.001.53924: GET /releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz HTTP/1.1 Host: 127.0.0.1:53924 User-Agent: Go-http-client/1.1 Referer: https://api.github.com/repos/mislav/will_paginate/assets/9876 X-Original-Port: 80 X-Original-Scheme: https Accept-Encoding: gzip 127.000.000.001.53929-127.000.000.001.53924: GET /releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz HTTP/1.1 Host: 127.0.0.1:53924 User-Agent: Go-http-client/1.1 Referer: https://api.github.com/repos/mislav/will_paginate/assets/9876 X-Original-Port: 80 X-Original-Scheme: https Accept-Encoding: gzip 127.000.000.001.53924-127.000.000.001.53929: HTTP/1.1 200 OK Content-Type: application/octet-stream Content-Length: 1238 Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53929: HTTP/1.1 200 OK Content-Type: application/octet-stream Content-Length: 1238 Server: WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18) Date: Wed, 17 Feb 2016 15:13:53 GMT Connection: Keep-Alive 127.000.000.001.53924-127.000.000.001.53929: { "GATEWAY_INTERFACE": "CGI/1.1", "PATH_INFO": "/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz", "QUERY_STRING": "", "REMOTE_ADDR": "127.0.0.1", "REMOTE_HOST": "localhost", "REQUEST_METHOD": "GET", "REQUEST_URI": "http://127.0.0.1:53924/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz", "SCRIPT_NAME": "", "SERVER_NAME": "127.0.0.1", "SERVER_PORT": "53924", "SERVER_PROTOCOL": "HTTP/1.1", "SERVER_SOFTWARE": "WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18)", "HTTP_HOST": "127.0.0.1:53924", "HTTP_USER_AGENT": "Go-http-client/1.1", "HTTP_REFERER": "https://api.github.com/repos/mislav/will_paginate/assets/9876", "HTTP_X_ORIGINAL_PORT": "80", "HTTP_X_ORIGINAL_SCHEME": "https", "HTTP_ACCEPT_ENCODING": "gzip", "rack.version": [ 1, 1 ], "rack.input": "#<StringIO:0x007fde2505e988>", "rack.errors": "#<IO:0x007fde240c6520>", "rack.multithread": true, "rack.multiprocess": false, "rack.run_once": false, "rack.url_scheme": "http", "HTTP_VERSION": "HTTP/1.1", "REQUEST_PATH": "/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz", "rack.logger": "#<Rack::NullLogger:0x007fde2510f120>", "rack.request.query_string": "", "rack.request.query_hash": { } } 127.000.000.001.53924-127.000.000.001.53929: { "GATEWAY_INTERFACE": "CGI/1.1", "PATH_INFO": "/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz", "QUERY_STRING": "", "REMOTE_ADDR": "127.0.0.1", "REMOTE_HOST": "localhost", "REQUEST_METHOD": "GET", "REQUEST_URI": "http://127.0.0.1:53924/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz", "SCRIPT_NAME": "", "SERVER_NAME": "127.0.0.1", "SERVER_PORT": "53924", "SERVER_PROTOCOL": "HTTP/1.1", "SERVER_SOFTWARE": "WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18)", "HTTP_HOST": "127.0.0.1:53924", "HTTP_USER_AGENT": "Go-http-client/1.1", "HTTP_REFERER": "https://api.github.com/repos/mislav/will_paginate/assets/9876", "HTTP_X_ORIGINAL_PORT": "80", "HTTP_X_ORIGINAL_SCHEME": "https", "HTTP_ACCEPT_ENCODING": "gzip", "rack.version": [ 1, 1 ], "rack.input": "#<StringIO:0x007fde2505e988>", "rack.errors": "#<IO:0x007fde240c6520>", "rack.multithread": true, "rack.multiprocess": false, "rack.run_once": false, "rack.url_scheme": "http", "HTTP_VERSION": "HTTP/1.1", "REQUEST_PATH": "/releases/12204602/22ea221a-cf2f-11e2-222a-b3a3c3b3aa3a.gz", "rack.logger": "#<Rack::NullLogger:0x007fde2510f120>", "rack.request.query_string": "", "rack.request.query_hash": { } }

Yeah seems that Go isn't handling 302 correctly. At first I thought that their HTTP client doesn't do redirects automatically at all, but now it turns out that it does, but messes it up. This might be a problem.

We should probably provide our own CheckRedirect implementation. Think you can take a stab at that?

@mislav I tried, but it doesn't seem to get called. afb17d8

EDIT: It is called during the cucumber tests...argh...

Interestingly, this worked with real GH releases for a week or so after I opened this PR. Today, it no longer works - probably because of the host header, though I'm not totally sure. I verified cURL has a correct host header while following the redirects.

Ok, fixed! Last issues with S3 was pebkac. S3 doesn't seem to care about the host header so far (probably because they don't use SNI). I also implemented redirect header preservation to match cURL behavior. It's not strictly required if you are concerned about it.

mislav · 2016-02-26T01:54:26Z

github/http.go

+		// Maintain headers after redirect.
+		// https://github.com/golang/go/issues/4800
+		for key, val := range via[0].Header {
+			if key == "Authorization" {


We should do a case-insensitive comparison here. Also, could we maintain even this header if the redirect is to the same host?

mislav · 2016-02-26T01:57:22Z

Awesome work on the redirect stuff! Still will come in handy for various different operations in the future 😸

glennpratt · 2016-02-26T14:15:33Z

@mislav addressed your comments.

owenthereal · 2016-02-26T15:26:22Z

commands/release.go

+	for _, asset := range release.Assets {
+		assetReader, apiError := gh.DownloadReleaseAsset(asset.ApiUrl)
+		utils.Check(apiError)
+		defer assetReader.Close()


We could close the assetReader on each loop is finished instead of waiting for the whole program.

Extracted to a function.

glennpratt · 2016-02-26T16:37:40Z

@jingweno I addressed your comments.

Add `hub release download` command that downloads all assets.

mislav · 2016-02-27T12:30:30Z

@glennpratt Thanks for all the hard work!

glennpratt · 2016-02-27T15:38:22Z

Woohoo. Thank you for all the guidance.

glennpratt force-pushed the release-download branch 2 times, most recently from 78d701c to a9c587e Compare February 6, 2016 02:58

glennpratt mentioned this pull request Feb 6, 2016

Add an option to download releases. #1102

Closed

glennpratt force-pushed the release-download branch from a9c587e to 2d761b8 Compare February 6, 2016 05:06

glennpratt force-pushed the release-download branch 2 times, most recently from 3c38b59 to 9331204 Compare February 6, 2016 05:13

mislav reviewed Feb 9, 2016
View reviewed changes

glennpratt force-pushed the release-download branch 2 times, most recently from 07e0675 to c5f8f78 Compare February 9, 2016 05:07

mislav reviewed Feb 10, 2016
View reviewed changes

glennpratt force-pushed the release-download branch from c5f8f78 to 5e15816 Compare February 14, 2016 04:30

mislav reviewed Feb 15, 2016
View reviewed changes

glennpratt force-pushed the release-download branch 2 times, most recently from 1c304d0 to 0769f95 Compare February 16, 2016 04:31

mislav reviewed Feb 16, 2016
View reviewed changes

glennpratt force-pushed the release-download branch 3 times, most recently from afb17d8 to c995586 Compare February 25, 2016 22:59

glennpratt force-pushed the release-download branch from c995586 to 22a4660 Compare February 25, 2016 23:18

mislav reviewed Feb 26, 2016
View reviewed changes

glennpratt force-pushed the release-download branch from 22a4660 to 8c09faf Compare February 26, 2016 14:15

owenthereal reviewed Feb 26, 2016
View reviewed changes

glennpratt force-pushed the release-download branch from 8c09faf to e63c2ac Compare February 26, 2016 16:36

Add hub release download command that downloads all assets.

c57cb62

glennpratt force-pushed the release-download branch from e63c2ac to c57cb62 Compare February 26, 2016 18:24

mislav added a commit that referenced this pull request Feb 27, 2016

Merge pull request #1103 from glennpratt/release-download

e046b8d

Add `hub release download` command that downloads all assets.

mislav merged commit e046b8d into mislav:master Feb 27, 2016

shouze mentioned this pull request Sep 2, 2019

hub release download - filtering and list #2251

Closed


		ui.Printf("Downloading %s ...\n", asset.Name)

		assetFile, err := os.OpenFile(asset.Name, os.O_WRONLY\|os.O_CREATE\|os.O_EXCL, 0600)

Add hub release download command that downloads all assets. #1103

Add hub release download command that downloads all assets. #1103

Conversation

glennpratt commented Feb 5, 2016

glennpratt commented Feb 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav commented Feb 9, 2016

glennpratt commented Feb 9, 2016

mislav commented Feb 9, 2016

glennpratt commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav commented Feb 15, 2016

glennpratt commented Feb 15, 2016

glennpratt commented Feb 16, 2016

glennpratt commented Feb 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav commented Feb 26, 2016

glennpratt commented Feb 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennpratt commented Feb 26, 2016

mislav commented Feb 27, 2016

glennpratt commented Feb 27, 2016

Add `hub release download` command that downloads all assets. #1103

Add `hub release download` command that downloads all assets. #1103