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

Add support for defining metadata #90

Merged
merged 2 commits into from
Jul 24, 2020

Conversation

msvaljek
Copy link
Contributor

I'm working on a project where I need to update the metadata to do something business relevant.

I thought about going around this one similar to how I'm already fetching the metadata i.e. by using execute but as you can see here this is only possible if I copy the file again, this is not optimal for my use-case.

This needs to be provided when creating the s3 object as seen here: https://stackoverflow.com/a/32646827/7413631

For now, I just added what I need, i.e. a map of metadata, possibly we could think about some quality tests/type safety.

@mijicd mijicd requested a review from regis-leray July 23, 2020 14:19
Copy link
Member

@mijicd mijicd left a comment

Choose a reason for hiding this comment

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

Looks good to me, up to @regis-leray to sign it off :)

@@ -130,7 +132,8 @@ package object s3 {
bucketName: String,
key: String,
contentType: String,
content: ZStream[R, Throwable, Byte]
content: ZStream[R, Throwable, Byte],
metadata: Map[String, String]
Copy link
Member

Choose a reason for hiding this comment

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

a default value would be great
metadata: Map[String, String] = Map.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.

@@ -169,7 +169,7 @@ object S3Suite {
val tmpKey = Random.alphanumeric.take(10).mkString

for {
_ <- putObject(bucketName, tmpKey, c.length.toLong, "text/plain", data)
_ <- putObject(bucketName, tmpKey, c.length.toLong, "text/plain", data, metadata = Map.empty)
Copy link
Member

Choose a reason for hiding this comment

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

also coud you add another test to verify metadata are set on the file

Copy link
Member

@regis-leray regis-leray left a comment

Choose a reason for hiding this comment

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

I add a few comments
Thank a lot for your contributions

@msvaljek
Copy link
Contributor Author

No problem @regis-leray, glad to help.

I'll definitely continue to contribute ;)

I added the defaults for the metadata 6a023d3.

For the metadata proper test it might involve a bit more work since the s3 is emulated with the file system. I would need to think of some smarter way how to store the metadata apart from the file, either a separate shadow bucket or file or something similar.

Given that it's just a pass of a value to the async client would it be possible that the version is released with the metadata support and I open a ticket to make the test since I have a deadline and I would really like to use the zio-s3 in my case and not use something else or roll my own stuff directly with the aws client?

@regis-leray
Copy link
Member

oki thanks. @msvaljek you will be able to use the snapshot release.
i want to do some cleanup on upload api.

thanks

@regis-leray regis-leray merged commit ad3b1d6 into zio:master Jul 24, 2020
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