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: support for encryption, storage class, custom headers #254

Merged
merged 6 commits into from
Apr 19, 2017

Conversation

kobmic
Copy link
Contributor

@kobmic kobmic commented Apr 7, 2017

Implemented server side encryption, added s3 storage class and custom headers. Fixes #109

Tried not to break the current S3Client multipartUpload API, even if I'm not to happy with always passing CannedAcl and MetaHeaders. Would be nicer to have Options for these arguments, i.e. always passing the default 'CannedAcl.Private' makes it impossible to add a custom 'x-amz-grant-read' header (conflicting).

@kobmic
Copy link
Contributor Author

kobmic commented Apr 11, 2017

Saw that Travis build of my PR failed with an error in AwsLambdaFlowSpec and that other PRs had the same issue - anything I can do to make Travis happy?

@johanandren
Copy link
Member

We're tracking that in #235, I guess someone re-triggered the build because this PR is green now.

@svezfaz
Copy link
Contributor

svezfaz commented Apr 14, 2017

@kobmic I agree, the S3Client API looks a bit brittle. I don't think we should go great length not to break it.. this should be all experimental ATM? Might be a good idea to change S3Client API now to use optional params.

@kobmic
Copy link
Contributor Author

kobmic commented Apr 14, 2017

If it's ok to change the current API I would refactor my PR, move the params to the new S3Header case class and make them optional. What do you think?

@svezfaz
Copy link
Contributor

svezfaz commented Apr 14, 2017

It makes sense to me. @johanandren WDYT?

@johanandren
Copy link
Member

I agree that this is better, but could we provide a @deprecated overload of the method that keeps the old signature? I'ts always unpleasant to have end user code break.

@kobmic
Copy link
Contributor Author

kobmic commented Apr 19, 2017

The compiler won't let me have multiple overloaded alternatives of method 'multipartUpload' with default arguments (which I still want in both cases). So I ended up with a 'multipartUploadWithHeaders' alternative without deprecating the old API.

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM!

@johanandren johanandren merged commit 3823145 into akka:master Apr 19, 2017
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