Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add support for creating downloads #97

Merged
merged 4 commits into from

5 participants

@catsby
Collaborator

:checkered_flag:

The specs pass, but this doesn't actually work yet. Creating a download is a two part process, the first part of which is simple enough. For the second part, I stubbed a POST call the the S3 url and created the hash for the body, and then implemented the method in downloads.rb to POST to that same URL. From what I can tell though in connection.rb lines 10-15 we essentially force all requests to go to github.com. That said, I'm confused why the "should post to a S3 url" spec passes.

In irb I've confirmed part one works. I rake install and run through it in irb, and although calling create_download does create a download on GitHub's site, I get a "false" response in irb and clicking on the created download from the Web UI gives a "file not found" message, so the post to S3 part is obviously failing.

So the two things here:

  • I'm planning on replacing line 42 of downloads.rb with a plain Faraday POST to the S3 url instead of our post shortcut. Is that a bad idea?
  • Why does that spec pass

Any feedback or guidance would be much appreciated.

@catsby catsby Add create_download method
The specs pass, but this doesn't actually work yet.
b953299
@pengwynn
Owner

I'm planning on replacing line 42 of downloads.rb with a plain Faraday POST to the S3 url instead of our post shortcut. Is that a bad idea?

I think that's the right approach since our shortcut is a helper to build API endpoint methods

Why does that spec pass?

I'm confused. If you've stubbed it, shouldn't it pass?

Also, I'm sure you already knew about it, but it took me a while to discover bundle console. It saves a ton of time compared to rake install + irb.

@catsby
Collaborator

I'm confused. If you've stubbed it, shouldn't it pass?

I was confused as to why the call was being made to https://github.s3.... when I had thought connection.rb and/or request.rb would force all urls to the https://api.github.com domain, but I guess I misunderstood what was happening in there.

it took me a while to discover bundle console

Thanks for sharing; I didn't know about that. Much better than what I was doing ;)

It also occurred to me that passing just a file name isn't going to cut it, we need either a full path or a ruby file object. In either case we should probably derive the size as well, so the method signatures would change to be something like

def create_download(repo, file_name, options={})

Where file_name could be a string or a file object. I'll try to work on this more tonight or this weekend. This branch is over at ctshryock/octokit:downloads if anyone stumbles on this and wants to take a stab at it.

@catsby catsby Re-work create_download to use Faraday and post to S3
Not working though, getting errors from S3
64d990d
@catsby
Collaborator

Getting closer here. Faraday and multipart-post handle the file stuff for me so it's fairly easy, however I think the documentation for uploading to S3 is missing a parameter, Expires:

AccessDeniedQuery-string authentication requires the Signature, Expires and AWSAccessKeyId parametersAAAA3798CF4046E5hDZLg8rzj7Pz7JTelSH9ssoAiVLkep7F/Q9/2CCzI221zIu4zUKmNFAmnUAzTX6L

Even after I add it to the POST hash I get a different error about the date being an invalid format

Invalid date (should be seconds since epoch): 4495266989

4495266989 being the value I got when I converted the expirationdate from part 1's response to 'time since epoch'.

I pushed what I have in case someone can spot what I'm doing stupid.

@pengwynn
Owner

Got pinged on this from a Python user, too. Works from curl. Digging...

@sigmavirus24

Yea, @ctshryock and I have been discussing this on IRC sporadically. It's very bizarre. Also, if it helps any, the original author of that section of the API was @tclem.

@catsby
Collaborator

I had an email drafted with my findings after digging more, but then tried it via curl before sending and it worked fine ಠ_ಠ

@tclem
Owner

It sucks that this is a 2 part API call :( I'd be glad to assit in any way possible. I did all my full stack testing with curl, so I know it works there as @ctshryock has noted. I also know that some of @kevinsawicki maven stuff uses this functionality so I'm sure we can make it work.

@tclem
Owner

Can you use something like HTTP Scoop or Fiddler to look at the differences between the requests coming from curl vs. those coming from octokit?

@kevinsawicki

I believe parameter order does matter and the GitHub Java API sets them in this order.

And I don't think you need to send the Expires parameter.

@catsby catsby Use the UploadIO object instead of the file name
Took the docs too literally and had "#file_name" when I should have been using
the UploadIO object, since this isn't curl :/
48e63e3
@catsby
Collaborator

Got this working with @pengwynn's help. User error on my part. The Expires param is not required. @sigmavirus24 Not sure if this helps you though

@sigmavirus24

Yeah I found a recipe on how to more properly create a form-data response. I'm not getting errors about that now, I'm getting a TimeOut error, not entirely sure why yet. At least it seems like I'm making some progress. Thanks anyway @ctshryock

@catsby catsby Wrap downloads feature
The spec could use some improvement.
859d5d3
@catsby
Collaborator

Ready to go

@pengwynn pengwynn merged commit 859d5d3 into octokit:master
@pengwynn
Owner

Thanks! I added support for delete and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 7, 2012
  1. @catsby

    Add create_download method

    catsby authored
    The specs pass, but this doesn't actually work yet.
Commits on Jun 13, 2012
  1. @catsby

    Re-work create_download to use Faraday and post to S3

    catsby authored
    Not working though, getting errors from S3
Commits on Jun 14, 2012
  1. @catsby

    Use the UploadIO object instead of the file name

    catsby authored
    Took the docs too literally and had "#file_name" when I should have been using
    the UploadIO object, since this isn't curl :/
Commits on Jun 18, 2012
  1. @catsby

    Wrap downloads feature

    catsby authored
    The spec could use some improvement.
This page is out of date. Refresh to see the latest.
View
33 lib/octokit/client/downloads.rb
@@ -24,6 +24,39 @@ def downloads(repo, options={})
def download(repo, id, options={})
get("repos/#{Repository.new(repo)}/downloads/#{id}", options, 3)
end
+
+
+ def create_download(repo, name, options={})
+ options[:content_type] ||= 'text/plain'
+ file = Faraday::UploadIO.new(name, options[:content_type])
+ resource = create_download_resource(repo, file.original_filename, file.size, options)
+
+ resource_hash = {
+ 'key' => resource.path,
+ 'acl' => resource.acl,
+ 'success_action_status' => 201,
+ 'Filename' => resource.name,
+ 'AWSAccessKeyId' => resource.accesskeyid,
+ 'Policy' => resource.policy,
+ 'Signature' => resource.signature,
+ 'Content-Type' => resource.mime_type,
+ 'file' => file
+ }
+
+ conn = Faraday.new(resource.s3_url) do |builder|
+ builder.request :multipart
+ builder.request :url_encoded
+ builder.adapter :net_http
+ end
+
+ response = conn.post '/', resource_hash
+ response.status == 201
+ end
+
+ private
+ def create_download_resource(repo, name, size, options={})
+ post("/repos/#{Repository.new(repo)}/downloads", options.merge({:name => name, :size => size}))
+ end
end
end
end
View
21 spec/fixtures/v3/download_create.json
@@ -0,0 +1,21 @@
+{
+ "url": "https://api.github.com/repos/octocat/Hello-World/downloads/1",
+ "html_url": "https://github.com/repos/octocat/Hello-World/downloads/new_file.jpg",
+ "id": 1,
+ "name": "new_file.jpg",
+ "description": "Description of your download",
+ "size": 1024,
+ "download_count": 40,
+ "content_type": ".jpg",
+ "policy": "ewogICAg...",
+ "signature": "mwnFDC...",
+ "bucket": "github",
+ "accesskeyid": "1ABCDEFG...",
+ "path": "downloads/ocotocat/Hello-World/new_file.jpg",
+ "acl": "public-read",
+ "expirationdate": "2011-04-14T16:00:49Z",
+ "prefix": "downloads/octocat/Hello-World/",
+ "mime_type": "image/jpeg",
+ "redirect": false,
+ "s3_url": "https://github.s3.amazonaws.com/"
+}
View
27 spec/octokit/client/downloads_spec.rb
@@ -7,7 +7,7 @@
@client = Octokit::Client.new(:login => 'sferik')
end
- describe ".downloads" do
+ describe ".getting downloads" do
it "should list available downloads" do
stub_get("/repos/github/hubot/downloads").
@@ -16,10 +16,6 @@
downloads.first.description.should == "Robawt"
end
- end
-
- describe ".download" do
-
it "should get a single download" do
stub_get("/repos/github/hubot/downloads/165347").
to_return(:body => fixture("v3/download.json"))
@@ -30,4 +26,25 @@
end
+ describe ".create a download" do
+ before(:each) do
+ stub_post("/repos/octocat/Hello-World/downloads").
+ with(:body => {:name => "download_create.json", :size => 690,
+ :description => "Description of your download",
+ :content_type => "text/plain" }).
+ to_return(:body => fixture("v3/download_create.json"))
+ end
+ it "should create a download resource" do
+ resource = @client.send(:create_download_resource, "octocat/Hello-World", "download_create.json", 690, {:description => "Description of your download", :content_type => "text/plain"})
+ resource.s3_url.should == "https://github.s3.amazonaws.com/"
+ end
+
+ it "should post to a S3 url" do
+ stub_post("https://github.s3.amazonaws.com/").
+ to_return(:status => 201)
+ file_path = File.expand_path 'spec/fixtures/v3/download_create.json'
+ @client.create_download("octocat/Hello-World", file_path, {:description => "Description of your download", :content_type => "text/plain"}).should == true
+ end
+ end
+
end
Something went wrong with that request. Please try again.