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

AWS S3: Custom header injection #1947

Merged
merged 11 commits into from
Oct 2, 2019
Merged

Conversation

vigoo
Copy link
Contributor

@vigoo vigoo commented Sep 24, 2019

Purpose

This PR adds the possibility to add custom headers to any of the AWS S3 operations.

References

The motivation was discussed in the following issue: #1938

Changes

  • Adds a new factory class beside the existing S3 called S3WithHeaders that contains the same set of operations but modified with the capability to pass an S3Headers value to any of them.

Background Context

I assumed (based on the contributor guidelines) that binary compatibility must be maintained, so I created a new API S3WithHeaders beside the existing one. If this would not be true, the contents of this new object could be the new S3 instead.

@lightbend-cla-validator

Hi @vigoo,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@vigoo
Copy link
Contributor Author

vigoo commented Sep 24, 2019

CLA signed

@ennru
Copy link
Member

ennru commented Sep 25, 2019

There are some unused imports in your code
https://travis-ci.com/akka/alpakka/jobs/238787362#L973

and this needs extra parens for Scala 2.13
https://travis-ci.com/akka/alpakka/jobs/238787363#L1249

@ennru
Copy link
Member

ennru commented Sep 26, 2019

Please add the new methods to the existing S3. Binary-compatiblity is provided even when adding new methods, as long as the old signature is available and the class is not intended for extension.

@vigoo
Copy link
Contributor Author

vigoo commented Sep 26, 2019

So I should rather add a xxxWithHeaders or something variant beside each existing method in S3?

@vigoo
Copy link
Contributor Author

vigoo commented Sep 26, 2019

Ok so I realized that they can be just overloaded methods, I'm pushing the change soon

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Looks much more regular now, great work.

I wonder if the S3Headers object should contain a val instance which to be returned where empty headers are required instead of creating a new every time.

@@ -140,7 +142,8 @@ import akka.util.ByteString
import mat.executionContext
implicit val conf = resolveSettings(attr, mat.system)

signAndGetAs[ListBucketResult](HttpRequests.listBucket(bucket, prefix, token))
// TODO: pass the headers
Copy link
Member

Choose a reason for hiding this comment

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

This comment is obsolete now, right?

@vigoo
Copy link
Contributor Author

vigoo commented Oct 2, 2019

I removed the obsolete comment and added an empty S3Headers value.

Comment on lines 127 to 129
val empty: S3Headers = S3Headers()

def apply() = new S3Headers()
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest you do it like:

val empty = new S3Headers()
def apply() = empty
def create() = empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer keeping the empty on the API too as I did or just hide it and use in apply and create?

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru changed the title [AWS S3] Custom header injection AWS S3: Custom header injection Oct 2, 2019
@ennru ennru merged commit 860ed58 into akka:master Oct 2, 2019
@ennru ennru added this to the 2.0.0 milestone Oct 2, 2019
@ennru
Copy link
Member

ennru commented Oct 2, 2019

Thank you, great work! We hope to see you around more.

@vigoo
Copy link
Contributor Author

vigoo commented Oct 2, 2019

Thanks for merging!

@vigoo vigoo deleted the s3-custom-header-injection branch October 2, 2019 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants