Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

aws s3 driver for Drive.put could return Response object, or the key. #52

Closed
Frondor opened this issue Jun 13, 2018 · 6 comments
Closed

Comments

@Frondor
Copy link

Frondor commented Jun 13, 2018

https://github.com/Slynova-Org/node-flydrive/blob/aea96ae1bc2995a4772391c3dc81682f9f63e500/src/Drivers/AwsS3.js#L107

Problem
Returning response.Location is limiting the app to know only that property of the result.
In cases where you're using different endpoints (for example, switching between s3 and minio servers for development), I'd rather use response.Key as the reference in DB, and then dynamically set the endpoint depending on the environment.

For example, putObject is doing it just fine
https://github.com/Slynova-Org/node-flydrive/blob/aea96ae1bc2995a4772391c3dc81682f9f63e500/src/Drivers/AwsS3.js#L163

Also, it could be considered to implement a common response object for all drivers, so you can expect always the same properties depending on which operation has been made, independently of the driver.
For instance: await Drive.put() could return a response object like

const response = await Drive.disk('s3').put(name, content, params)
response.location // full url (can be empty for local storage)
response.path // same as aws s3 response.Key - /bucket/prefix/file.jpg
response.status // 200
response.response // underlying response object if exists
@RomainLanz
Copy link
Member

I like the idea!

This package need some rewrite to be sure that output of all function are consistent (this is why #35 is still not merge btw).

If you wanna help I'd be happy to accept a PR 👍

@Frondor
Copy link
Author

Frondor commented Jun 14, 2018

Will start working on it next weekend 👍

@Frondor
Copy link
Author

Frondor commented Jul 2, 2018

Hello, I'll put this "on hold" on my part until a solution for a compact testing environment is found (docker image?)
Should I open a separate issue to track that?

@RomainLanz
Copy link
Member

Should I open a separate issue to track that?

That would be awesome! Do you have any documentation about testing with a Docker Image?

@Frondor
Copy link
Author

Frondor commented Jul 8, 2018

Sure, I'll create a separate issue for this

@RomainLanz RomainLanz added this to the 1.0.0 milestone Mar 17, 2019
@RomainLanz
Copy link
Member

PR for this: #104

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants