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

New Storage Driver: Minio #962

Merged
merged 38 commits into from
Sep 5, 2023
Merged

New Storage Driver: Minio #962

merged 38 commits into from
Sep 5, 2023

Conversation

mstgnz
Copy link
Contributor

@mstgnz mstgnz commented Aug 23, 2023

Description

Hello Fiber Team,

With this pull request, I'm introducing the MinIO S3 driver to Fiber's array of storage drivers. MinIO, a high-performance and scalable object storage system designed for modern cloud-native applications, brings exceptional speed and resilience.

Why MinIO?

MinIO is an ideal solution for projects that demand speed, durability, and scalability, making it particularly suitable for cases with significant data loads and high traffic. By incorporating this solution into the Fiber project, we can better address larger workloads and handle intensive traffic scenarios.

How Can This Help?

This pull request adds the MinIO S3 driver integration. I've expanded the existing test coverage by adding new test scenarios and documented how to integrate using sample code.

Please review and share your feedback. I'm open to any changes or suggestions for improvements. I'm excited to contribute to the Fiber project and believe that the MinIO integration can provide valuable advantages to our community.

@ReneWerner87
Copy link
Member

ReneWerner87 commented Aug 23, 2023

thx for the effort
@mstgnz
pls check the missing tasks

  • Adapt the readme in the root directory for the new adapter
  • adding the test workflow
  • adding the release drafter workflow and config
  • add the dependabot part for the new adapter/driver
  • add the new adpater for the security check in the workflow
  • check all other workflows for integration with the other adapters

before we start with the code review

minio/go.mod Outdated
github.com/klauspost/compress v1.16.7 // indirect
github.com/klauspost/cpuid/v2 v2.2.5 // indirect
github.com/minio/md5-simd v1.1.2 // indirect
github.com/minio/minio-go/v7 v7.0.62 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

why is it marked as indirect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

auto-attached was not my choice

@ReneWerner87
Copy link
Member

currently not sure what the difference is between the current s3 storage adapters and these if the underlying endpoints are the same

what added value does this implementation offer compared to the current s3 adapter?

@mstgnz
Copy link
Contributor Author

mstgnz commented Aug 23, 2023

currently not sure what the difference is between the current s3 storage adapters and these if the underlying endpoints are the same

what added value does this implementation offer compared to the current s3 adapter?

The current s3 adapter is written for aws, aws s3 is paid, minio is an open source project.

https://github.com/minio/minio

@ReneWerner87
Copy link
Member

ok thx for the explanation

@gaby
Copy link
Member

gaby commented Aug 23, 2023

currently not sure what the difference is between the current s3 storage adapters and these if the underlying endpoints are the same
what added value does this implementation offer compared to the current s3 adapter?

The current s3 adapter is written for aws, aws s3 is paid, minio is an open source project.

https://github.com/minio/minio

S3 is a protocol though, you can use the s3 driver with Minio. We even use Minio to run the s3 driver unit-tests. See here:

https://github.com/gofiber/storage/blob/main/.github/workflows/test-s3.yml#L24

@gaby
Copy link
Member

gaby commented Aug 23, 2023

I was looking around and it's more simple than the official S3 one. So it makes sense

README.md Outdated Show resolved Hide resolved
@ReneWerner87
Copy link
Member

pls refresh with the master
42ff8d5

minio/README.md Show resolved Hide resolved
minio/README.md Show resolved Hide resolved
minio/go.mod Outdated Show resolved Hide resolved
minio/go.mod Outdated Show resolved Hide resolved
minio/minio.go Show resolved Hide resolved
minio/minio.go Show resolved Hide resolved
minio/minio_test.go Outdated Show resolved Hide resolved
minio/minio_test.go Outdated Show resolved Hide resolved
minio/minio_test.go Outdated Show resolved Hide resolved
minio/minio_test.go Outdated Show resolved Hide resolved
@gaby
Copy link
Member

gaby commented Aug 26, 2023

I have to re-review the unit-tests. Overall it looks fine. I'm just a little concern with the extra Public functions allowing the user to Create and Remove buckets. This is very to the behavior of the S3 driver.

Does Minio let you upload data as bytes or strings? Instead of having to upload "files"?

@mstgnz
Copy link
Contributor Author

mstgnz commented Aug 26, 2023

I have to re-review the unit-tests. Overall it looks fine. I'm just a little concern with the extra Public functions allowing the user to Create and Remove buckets. This is very to the behavior of the S3 driver.

Does Minio let you upload data as bytes or strings? Instead of having to upload "files"?

Minio PutObject requests io.Reader just like s3 PutObject.

@mstgnz
Copy link
Contributor Author

mstgnz commented Aug 26, 2023

Why use Minio Client over S3 Sdk?

Since Minio Client implements the Amazon S3 API, it can work not only with Amazon S3 but also with other Amazon S3-compatible object storage solutions. This can enhance the customizability and independence of your project. If you have the possibility of transitioning to a different compatible object storage solution in the future, using Minio Client could make this transition smoother.

Minio Client can address a wider range of use case scenarios beyond just Amazon S3. It gives you the ability to target different compatible storage servers or services, not limited to Amazon S3.

Minio Client is a tool developed and supported in alignment with open source projects. This could also be seen as a way to contribute to the development and improvement of the open source community.

Minio Client might offer a user-friendly interface to enhance developer experience. Especially when working with a Minio server, tasks performed through Minio Client could be faster and more straightforward.

If you have developers familiar with Minio Client usage on your team or if your team better understands Minio Client, this could influence your choice. Similarly, if there's already a pre-existing infrastructure based on Minio Client usage, it could be a factor to consider.

If you are building a project specifically tailored to work on a Minio server, using Minio Client could provide a more direct and integrated way of working with this server.

@mstgnz mstgnz requested a review from gaby August 26, 2023 14:39
@gaby
Copy link
Member

gaby commented Aug 26, 2023

@mstgnz Can we get rid of a the "CheckBucket" calls on each function. When the call happens if the bucket doesnt exist it should throw an error anyways. It's up to the user to check the error and create the bucket.

@mstgnz
Copy link
Contributor Author

mstgnz commented Aug 26, 2023

OK, I'll have to adjust the test accordingly.

edit: bucket check control for get, set, delete and reset has been removed. And update test @gaby

minio/config.go Outdated Show resolved Hide resolved
minio/minio.go Outdated Show resolved Hide resolved
@mstgnz mstgnz requested review from efectn and ReneWerner87 August 29, 2023 15:54
minio/minio.go Outdated Show resolved Hide resolved
@mstgnz mstgnz requested a review from efectn August 31, 2023 11:06
@ReneWerner87 ReneWerner87 merged commit f9a8727 into gofiber:main Sep 5, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants