search

feathersjs-ecosystem / feathers-blob

Feathers service for blob storage, like S3.
http://feathersjs.com
MIT License
92 stars 32 forks source link

feat(s3): add support for fetching versioned bucket items #88

Closed green3g closed 3 years ago

green3g commented 3 years ago

Summary

This pull request adds $versionId as a supported query parameter to allow clients to request a specific version of a key in S3. It does so by mixing in the S3 parameters with the query paramter (if provided) to provide VersionId to the s3 api.

(If you have not already please refer to the contributing guideline as described here)

If so, please mention them to keep the conversations linked together.

Other Information

If there's anything else that's important and relevant to your pull request, mention that information here. This could include benchmarks, or other information.

Your PR will be reviewed by a core team member and they will work with you to get your changes merged in a timely manner. If merged your PR will automatically be added to the changelog in the next release.

If your changes involve documentation updates please mention that and link the appropriate PR in feathers-docs.

Thanks for contributing to Feathers! :heart:

claustres commented 3 years ago

Thanks for this PR.

My suggestion would be to remove the $ prefix, which is usually reserved for built-in verbs to be interpreted by adapters and to keep the AWS naming as is by using VersionId in the query so that we will be in line with the AWS documentation. Otherwise, we will also need to make a "transformation" each time we will add a new parameter, which is error-prone IMHO.

Last but not least, could you please add a test for this? Since we migrated from TravisCI to GitHub workflow the S3 tests are not run in CI but you can run them on your local environment by using the following environment variable S3_BUCKET to set the target bucket.

Thanks again.

green3g commented 3 years ago

Sure thing! Thanks for the feedback. I'll revise and re-push.

claustres commented 3 years ago

Seems to me ok, thanks

green3g commented 3 years ago

Thanks for merging - any ETA on release for a new version?

claustres commented 3 years ago

Done as v2.6