scttnlsn / dandelion

Incremental Git repository deployment.
http://scttnlsn.github.io/dandelion
MIT License
738 stars 57 forks source link

s3 backend #2

Closed ahamid closed 13 years ago

ahamid commented 13 years ago

for your consideration. requires aws-s3 gem for optional s3

scttnlsn commented 13 years ago

This is awesome!

I'd love to keep adding more backends but I am a little reluctant to have so many (often unnecessary) dependencies. In general, I'm wondering if it might be better to defer requiring any "transport" gems (net/sftp, net/ftp, aws-s3, etc.) until the corresponding service class is initialized. There could potentially be an additional Dandelion command to install any gems required by the specified "scheme".

Any thoughts here? I'm new to the Ruby world and unsure how this sort of thing is typically handled.

Thanks!

-Scott

ahamid commented 13 years ago

Hi Scott, the standard way to add a dependency library is through the gem's gemspec. I specifically did not add aws-s3 there, considering it an 'optional' dependency that the user could 'gem install' themselves. It may be possible to programatically install ruby gems on-demand to make it easier for the user, but i've never tried this myself. The convention I've seen is that it's up to the user to install any "optional" dependencies. You could wrap the "require 'aws-s3'", catch LoadError, and print out a more friendly message: "The aws-s3 gem is required to use the backend. Please install the gem: gem install aws-s3".

scttnlsn commented 13 years ago

Cool, thanks. I'll merge this and then move in that direction.

scttnlsn commented 13 years ago

Is line 202 of lib/dandelion/service.rb (https://github.com/incandescent/dandelion/commit/64179bc399ea205029e0da65b30767d001d73207) supposed to be "#{@path}/#{file}" instead of "#{@path}/file"? I'm totally unfamiliar with S3 but my guess is that this is a typo.

I'll get mock-aws-s3 (https://github.com/jkrall/mock-aws-s3) going and test this out.

Thanks!

-Scott

ahamid commented 13 years ago

Oops, yes you are right Scott. I think i factored out that function after some initial testing and didn't test a path prefix again :( my bad