add a github downloader #947
Conversation
|
I don't really think this belongs in berkshelf core - is there a reason this isn't part of the API server? |
|
@sethvargo @punkle is right, Berkshelf needs to know how to download each @punkle this looks really solid, my comments are inline |
| @@ -65,6 +70,24 @@ def try_download(source, name, version) | |||
| # https://github.com/opscode/test-kitchen/blob/master/lib/kitchen.rb#L99 | |||
| Celluloid.logger = nil unless ENV["DEBUG_CELLULOID"] | |||
| Ridley.open(credentials) { |r| r.cookbook.download(name, version) } | |||
| when :github | |||
| tmp_dir = "/tmp/#{name}/#{version}" | |||
| url = URI("https://codeload.github.com/#{remote_cookbook.location_path}/legacy.tar.gz/v#{version}") | |||
There was a problem hiding this comment.
Why not just use the standard archive link (ie. https://github.com/punkle/berkshelf/archive/v2.0.10.tar.gz)? I admittedly don't even know what this legacy one is.
There was a problem hiding this comment.
Hey, I can't remember exactly where I found that url but I imagine I had followed a resource href in the github api somewhere. The url you are suggesting is much cleaner. With your url the root of the tar.gz contains the actual repository. This is not the case with the codeload url. Hence the complicated fname.include?("-#{name}-") malarkey. I can improve on this, in the morning (GMT).
|
@sethvargo also, the downloading logic should be moved into the berkshelf-api-client gem in the near future. That should clean stuff up a bit. |
| @@ -65,6 +70,24 @@ def try_download(source, name, version) | |||
| # https://github.com/opscode/test-kitchen/blob/master/lib/kitchen.rb#L99 | |||
| Celluloid.logger = nil unless ENV["DEBUG_CELLULOID"] | |||
| Ridley.open(credentials) { |r| r.cookbook.download(name, version) } | |||
| when :github | |||
| tmp_dir = "/tmp/#{name}/#{version}" | |||
There was a problem hiding this comment.
Temp directories should be created with Dir.mktmpdir. Don't forget to cleanup afterwards! :)
There was a problem hiding this comment.
Where should this tmp dir be cleaned up? The try_download function returns a path to the tmp directory. If I delete the tmp directory at that point, won't it fail?
- better github download url - using Dir.mktmpdir - removes include Archive::Tar
|
@punkle thanks for doing both ends of this |
|
The berkshelf-api's github downloader uses an oauth token to communicate with github. This implies that you would be able to index private repositories. But after looking at the downloader code here I am thinking that it would not be able to download private repositories. Is that true? |
|
@rteabeault good catch, we'll need to refactor this downloader a bit before 3.0 comes out. |
This is to support downloading cookbooks from a github location type by the berkshelf-api provided by Issue https://github.com/berkshelf/berkshelf-api/issues/27