Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for getting files from S3 #2

Closed
dancannon opened this issue Oct 22, 2015 · 6 comments
Closed

Add support for getting files from S3 #2

dancannon opened this issue Oct 22, 2015 · 6 comments

Comments

@dancannon
Copy link
Contributor

We are looking into using Nomad and store our binaries in S3. To fetch these as part of the job startup it would be helpful for go-getter to support S3.

I have started this here but I am not sure about my approach. I would be interested to hear your thoughts.

@catsby
Copy link
Contributor

catsby commented Oct 22, 2015

Hey @dancannon – I imagine S3 is certainly a thing we'd like to support. Unless you feel it will be a lot of work, go ahead and get it working and then open a pull request and we'll be happy to review and provide suggestions!

I did look at what you have and it seems straightforward enough. Do you have any ideas on how you plan on handling authentication? Defaulting to the aws-sdk-go to find them in ~/.aws/credentials or in the environment? That sounds sufficient for me, but maybe others have an opinion

@dancannon
Copy link
Contributor Author

Hey @catsby thats great to hear. Regarding my branch:

  • Providing the Get function might be a little difficult for S3 unless I have misunderstood what it is meant to be doing.
  • I thought that relying on aws-sdk-go for authentication would be enough but if not I imagine that credentials could be passed to the S3Getter to override the defaults, how does that sound?

@dancannon
Copy link
Contributor Author

Also what did you think about my method for matching S3 URLs? My current attempt will cause any S3 urls to use the S3 getter and not the HTTP getter, I dont think this is a bad thing but what do you think?

@mitchellh
Copy link
Contributor

Hey @dancannon. What you're starting looks great! I've put some points below in no particular order to help you out:

  • All of go-getter is unit tested quite well. Take a look at how we test everything and try to copy! In particular, use table tests for detectors so we can see how you're detecting URLs.
  • For auth, I think we should use all the defaults for the aws-sdk-go SDK but also probably allow for them from URL query params (that we parse out).
  • For Get, we should probably list the prefix and download all the files. I think S3 can do this quite well.
  • Please write tests even for the Get methods using some publicly available S3 URL. We can replace this with our own S3 URL on merge so your account isn't on the line!

Let me know if you have any other questions and we'd love to have this in.

@mitchellh
Copy link
Contributor

Done, by you!

@dancannon
Copy link
Contributor Author

Thanks for reviewing + merging!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

3 participants