-
Notifications
You must be signed in to change notification settings - Fork 73
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 content type user supplied #4895
S3 content type user supplied #4895
Conversation
.../ch/epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/s3/S3FileOperations.scala
Outdated
Show resolved
Hide resolved
6106f74
to
26ba43b
Compare
private def parseContentType(raw: String): IO[ContentType] = | ||
ContentType.parse(raw).map(_.pure[IO]).getOrElse(IO.raiseError(InvalidContentType(raw))) | ||
private def parseContentType(raw: Option[String]): IO[Option[ContentType]] = { | ||
raw match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
traverse could be your friend here
|
||
def baseEndpoint: Uri | ||
} | ||
|
||
object S3StorageClient { | ||
|
||
case class UploadMetadata(checksum: String, fileSize: Long, location: Uri) | ||
case class HeadObject( | ||
fileSize: Long, | ||
contentType: Option[String], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could have directly a ContentType
here
case class HeadObject( | ||
fileSize: Long, | ||
contentType: Option[String], | ||
sha256Checksum: Option[String], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same it could be a Digest
.../ch/epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/s3/S3FileOperations.scala
Outdated
Show resolved
Hide resolved
26ba43b
to
bf5af4e
Compare
@imsdu I addressed one comment, but currently in merge hell and not sure about the others. I don't disagree with what you're saying I'm just going to avoid dealing with it for now, because they aren't blocking us |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, probably some refactoring to do separately - would be good to have this merged so I can deal with the conflicts in the update PR too
Fixes #4881