Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Added a custom key function in server side #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bensu
Copy link
Contributor

@bensu bensu commented May 25, 2015

This is a conflict free version of #6 . For convenience:

Users can now provide a :key-fn to the server handler which given file-name and mime-type will determine the key to use for the S3 file.

Caveat: if the client provides a file my-file.txt and the :key-fn generates a random number as a key (i.e. 12314345), the file location in S3 is given by 12314345, which is what was passed back in the s3-pipe uploaded channel. The file location is no longer sufficient for the client to know which file was successfully uploaded (in case of parallel uploads).

My (non-backwards-compatible) proposal is to put in the uploaded channel all information relevant to the client: both the file object that was uploaded and the response from S3 which contains the :key, :location, :bucket, and :etag.

@martinklepsch
Copy link
Owner

@bensu Two small things:

  1. Can you make the commit message a bit more descriptive? (you can just do git commit --amend and force push if you like.)
  2. Also the changelog mentioning the incompatible change between message formats would be nice.

@bensu
Copy link
Contributor Author

bensu commented May 25, 2015

@martinklepsch sure!

  1. I forgot about the commit message when trying to fix the conflicts.
  2. Let me know if what I added to the Changelog is what you had in my mind.

@martinklepsch
Copy link
Owner

@bensu Do I understand correctly that you'll need to provide your own s3-sign ring handler to supply the key function? Maybe we want to remove the s3-sign route completely and just provide an example in the readme?

In any case adding a usage example would be nice I think.

Sorry for the delayed reply :)

@ldenman
Copy link

ldenman commented Aug 24, 2015

Hey @martinklepsch & @bensu ,

I took some time and worked to get this patch up to date with master. I also removed the s3-sign route and updated the readme with an example of using a custom key. Let me know what you think, please at #16 . Cheers!

@martinklepsch martinklepsch mentioned this pull request Sep 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants