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

S3 object options #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

S3 object options #6

wants to merge 4 commits into from

Conversation

kaapa
Copy link

@kaapa kaapa commented Jun 8, 2015

Issue #3

Provides server_side_encryption and storage_class configuration options.

I was unable to get the test suite to pass tests regarding non-existing files, but this was from the get go, so I don't believe my changes caused that regression.

PS. If this approach is acceptable, but you'd like a nicer commit history I can squash the commits and create a new PR.

@grosser
Copy link
Contributor

grosser commented Jun 17, 2015

@jnicklas

end

object(id).send(operation, s3_options_for(operation, options))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bit too clever, and I'm not really sure why it's needed. Can't we just add the options to how it was before?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous version with AWS SDK V 1.x it was possible to pass these options as defaults to the client object whereas now they are only available as options for the "operation" methods (copy_from, put, presigned_post).

presigned_post and copy_from/put have slightly different APIs regarding their options (as described in #3). The two options implemented here happen to share same option keys across the three operations, but for example options to provide custom encryption keys seem to have different hash keys in presigned_post than in other two.

So the indirection is there so that it'll be simple to add the other keys from the respective operation APIs when someone needs them. If supporting those options is over-architecting at this point then a simpler implementation is indeed possible.

@kaapa
Copy link
Author

kaapa commented Jul 9, 2015

Ping! Any change to elaborate on how you'd like to see this one simplified so it'd get merged? I'd love to see this feature implemented whether it's via this PR or some other way, but I'm out of ideas how to push this forward without a bit of guidance... Thank you! :)

@fran-worley
Copy link

I agree with the thought process behind this but it strikes me that it only goes part way to solving the issue. The following come to mind currently:

  1. Additional options: whilst easy to add using your final code, it would require either whitelisting all available options for each S3 method used, or a PR every time someone has a new one. In this case, your first approach works better as it's more flexible inspite of the duplicated keys.
  2. Some of these settings should be definable per model or even per model attachment class.
    e.g. Picture.image might have a public acl, and be unencrypted but SecureDocument.backup may be the opposite. I guess implement this would require some changes to 'refile/refile' to ensure that the params are filtered through the attachment method correctly.

👍 on the PR, would be great to hear thoughts on the above and try to get something merged soon.

@counterbeing
Copy link

counterbeing commented Jul 22, 2016

👍 This does seem like a good solution to the problem.

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.

5 participants