S3: Bucket management #1608

MarcinAman · 2019-03-25T15:59:43Z

Pull Request Checklist

Have you read through the contributor guidelines and contributor advice?
Have you signed the Lightbend CLA?
Have you updated the documentation?
Have you added tests for any changed functionality?

Fixes

Purpose

What does this PR do?
Regarding the issue that i posted i've created some bucket management functionality. It allows end user to create, delete and check if bucket of a given name exists.

As for now the docs created by AWS doesn't specify a request to list all buckets so that functionality was not implemented.

Note

I'd be glad if someone would check whether the naming in the enum is understandable for others and is there any better solution to have the same request but with different mapping functions (i am talking about explicitly passing this implicit in S3Stream to processing functions)?

ennru · 2019-03-26T08:49:00Z

Thank you for this PR!

It would be more natural to expose this functionality as Futures/CompletionStages.
Instead of the Enumeration, please prefer a sealed trait with case objects.

MarcinAman · 2019-03-26T09:19:44Z

@ennru Could you explain why it would be more natural to use Futures/CompletionStages rather than Sources?

ennru · 2019-03-26T09:51:24Z

For asynchronous work that has at most a single result, futures express that better. As you see in your tests, all you'd normally do is to run the source with a Sink.headOption (or Sink.ignore which gives the same semantics).
But maybe there is some useful application of the sources when composing it with other S3 streams?

MarcinAman · 2019-03-27T12:34:33Z

I see the point, but while writing it i was looking also on method:

def deleteObject(bucket: String, key: String, versionId: Option[String] = None): Source[Done, NotUsed]

I believe that makeBucket and deleteBucket signature should be the same to avoid confusion.

As for the checkIfBucketExits it is nice when it is a source. Ok, it always has only one value but i can imagine a use case when somebody would compose that information with other Sources. For example something similar to this would be a valid use case. User checks whether the bucket exits and if not creates it.

ennru

We can keep the Source versions and add Future as an alternative.
Please add these new great features to the docs, as well. (For that you'll need some Java snippets and can try out how it looks from Java.)

ennru · 2019-04-01T11:53:36Z

s3/src/main/scala/akka/stream/alpakka/s3/impl/S3Stream.scala

+
+    response match {
+      case HttpResponse(status, _, entity, _) if status.isSuccess() =>
+        Source.fromFuture {


This wrapping of Futures into Sources should be removed, a mapAsync is much leaner than flatMapConcat.

ennru · 2019-04-01T11:54:10Z

s3/src/main/scala/akka/stream/alpakka/s3/javadsl/S3.scala

@@ -324,7 +324,7 @@ object S3 {
   * @param key the s3 object key
   * @param contentType an optional [[akka.http.javadsl.model.ContentType ContentType]]
   * @param s3Headers any headers you want to add
-   * @return a [[akka.stream.javadsl.Sink Sink]] that accepts [[akka.util.ByteString ByteString]]'s and materializes to a [[ava.util.concurrent.CompletionStage CompletionStage]] of [[MultipartUploadResult]]
+   * @return a [[akka.stream.javadsl.Sink Sink]] that accepts [[akka.util.ByteString ByteString]]'s and materializes to a [[java.util.concurrent.CompletionStage CompletionStage]] of [[MultipartUploadResult]]


ennru · 2019-04-01T12:06:07Z

s3/src/main/scala/akka/stream/alpakka/s3/model.scala

+ */
+sealed trait BucketAccess
+
+case object AccessDenied extends BucketAccess


For Java-friendliness:

change BucketAccess from trait to class

Put the objects in object BucketAccess

Add values in the BucketAccess object (val accessDenied: BucketAccess = AccessDenied)

ennru · 2019-04-01T12:07:17Z

s3/src/test/scala/akka/stream/alpakka/s3/scaladsl/S3IntegrationSpec.scala

+
+    val request: Future[Option[Done]] = S3
+      .makeBucket(bucketName)
+      .runWith(Sink.headOption)


Is there anything the Option expresses? Doesn't Sink.ignore say everything?

ennru

Looks good now! Most importantly exists is spelt wrong in a few places.

docs/src/main/paradox/s3.md

s3/src/main/scala/akka/stream/alpakka/s3/javadsl/S3.scala

s3/src/main/scala/akka/stream/alpakka/s3/scaladsl/S3.scala

Co-Authored-By: MarcinAman <marcin.aman@gmail.com>

ennru · 2019-04-26T13:34:04Z

Please note that there is a tiny conflict with master.

MarcinAman · 2019-04-26T13:37:59Z

Yes, i know. Currently i am working on finding more typos and this match statement

MarcinAman · 2019-04-29T12:55:16Z

@ennru I think i am quite happy with current state of this code. Could have a "final" look?

ennru · 2019-04-30T10:09:44Z

s3/src/main/scala/akka/stream/alpakka/s3/scaladsl/S3.scala

+   * @param bucketName bucket name
+   * @return [[scala.concurrent.Future Future]] with type [[Done]] as API doesn't return any additional information
+   */
+  def makeBucket(bucketName: String)(implicit mat: Materializer, attr: Attributes = Attributes()): Future[Done] =


I'm a bit surprised the overload works for picking the correct method based on the return type. And I wonder if it would be better to come up with different names to properly distinguish them.

I mean it is kind of good and bad. It is consistent and clear but only works when you specify type.

Do you think that it would be better when Source version would be makeBucket and future versions: makeBucketF?

I think I'd prefer to append Source.

def makeBucketSource(bucketName: String): Source[Done, NotUsed]

looks OK, doesn't it?

Makes sense 👍

I will submit a updated version in few hours

ennru

LGTM.

zanella · 2019-05-07T18:26:06Z

docs/src/main/paradox/s3.md

+For more information about attributes see: @scaladoc[S3Attributes](akka.stream.alpakka.s3.S3Attributes$) and @scaladoc[Attributes](akka.stream.Attributes)
+
+### Make bucket
+In order to create a bucket in S3 you need to specify it's unique name. This value has to be set accordingly to the [requirements](https://docs.aws.amazon.com/awscloudtrail/latest/userguide/cloudtrail-s3-bucket-naming-requirements.html).


The link seems to be the wrong one (cloudtrail), guess this is the right one ? -> https://docs.aws.amazon.com/AmazonS3/latest/dev/BucketRestrictions.html#bucketnamingrules

Yes, thank you for noticing. My bad on not checking it before merging. I will submit another PR tomorrow referencing that issue

MarcinAman force-pushed the bucket_management branch from e336091 to edca02f Compare March 25, 2019 21:46

ennru changed the title ~~Bucket management~~ S3: Bucket management Mar 26, 2019

ennru added the p:aws-s3 label Mar 26, 2019

ennru reviewed Apr 1, 2019

View reviewed changes

ennru reviewed Apr 26, 2019

View reviewed changes

MarcinAman added a commit to MarcinAman/alpakka that referenced this pull request Apr 26, 2019

Mostly fixing typos akka#1608

1d6187d

Co-Authored-By: MarcinAman <marcin.aman@gmail.com>

MarcinAman force-pushed the bucket_management branch 5 times, most recently from d7fd3d2 to 699c592 Compare April 26, 2019 17:08

ennru reviewed Apr 30, 2019

View reviewed changes

MarcinAman force-pushed the bucket_management branch from 79fae88 to 981df23 Compare May 7, 2019 11:14

Creating and deleting bucket. Checking if it exists akka#1546

981df23

ennru approved these changes May 7, 2019

View reviewed changes

ennru merged commit 9dc2766 into akka:master May 7, 2019

zanella reviewed May 7, 2019

View reviewed changes

MarcinAman mentioned this pull request May 8, 2019

S3: Broken bucket naming rules link #1684

Merged

3 tasks

ennru added this to the 1.0.1 milestone May 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3: Bucket management #1608

S3: Bucket management #1608

MarcinAman commented Mar 25, 2019

ennru commented Mar 26, 2019

MarcinAman commented Mar 26, 2019

ennru commented Mar 26, 2019

MarcinAman commented Mar 27, 2019

ennru left a comment

ennru Apr 1, 2019

ennru Apr 1, 2019

ennru Apr 1, 2019

ennru Apr 1, 2019

ennru left a comment

ennru commented Apr 26, 2019

MarcinAman commented Apr 26, 2019

MarcinAman commented Apr 29, 2019

ennru Apr 30, 2019

MarcinAman Apr 30, 2019

ennru May 7, 2019

MarcinAman May 7, 2019

ennru left a comment

zanella May 7, 2019

MarcinAman May 7, 2019

S3: Bucket management #1608

S3: Bucket management #1608

Conversation

MarcinAman commented Mar 25, 2019

Pull Request Checklist

Fixes

Purpose

Note

ennru commented Mar 26, 2019

MarcinAman commented Mar 26, 2019

ennru commented Mar 26, 2019

MarcinAman commented Mar 27, 2019

ennru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ennru left a comment

Choose a reason for hiding this comment

ennru commented Apr 26, 2019

MarcinAman commented Apr 26, 2019

MarcinAman commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ennru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment