-
Notifications
You must be signed in to change notification settings - Fork 497
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
adding support for publishing bundle to Minio Object Storage (#5395) #5757
base: main
Are you sure you want to change the base?
adding support for publishing bundle to Minio Object Storage (#5395) #5757
Conversation
Thanks for the contribution @Dimss! I'll try to take a look at it this week. |
@Dimss Minio is API compatible with AWS S3 API, so it should be possible to use the existing |
@sorindumitru I think the main question here, should it be a generic S3 publisher or publisher for each provider with it's backend implementation. |
Thank you so much for working on this! :) Its using the minio client, but I think it could work against any s3 server as coded? There are a lot of s3 compatible implementations out there (many storage vendors support it) and they all work with the aws s3 client I believe, or one of the implementations like minio's client. Having one generic s3 driver I think would be preferable to one per storage implementation, as they all essentially work the same. |
Having separate implementations for different platforms may make sense eventually, but I feel that for now it only adds maintenance burden. If someone does end up needing something from this plugin where it makes sense to have it use platform specific APIs, such as platform specific authentication methods, we should look into splitting off that part into a separate plugin. @Dimss, do you need anything from an S3 publisher that is Minio specific? For now, it's probably easiest to just add support for specifying an optional endpoint to the awss3 plugin. |
@sorindumitru ok by me. |
Just endpoint for now I think? if you need to load in a custom ca for talking to minio, it can be done via the system trust bundle. If its actually insecure, not sure there is any advantage to funneling it through the s3 service rather then just using the agents |
@kfox1111 agree. then I'll just add one optional |
@kfox1111, @sorindumitru I applied required changes, pls review. |
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.
Thank you @Dimss for this contribution!
We need to update the documentation of the aws_s3 bundle publisher plugin with the new Endpoint setting, indicating that's an optional setting (not required).
We should also add test coverage for this, making sure to check that the configuration has been properly wired up to the aws config.
Hi @Dimss, I think we’re pretty close to having this merged. Have you had a chance to look at the latest small comments? |
Hey @amartinezfayo yeah, I saw the comments, I'll make it done in coming days. |
@amartinezfayo I've applied required changes, added tests and updated docs. |
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. Thanks! :)
Looks like the DCO needs fixing. |
67a371d
to
a4ad9d1
Compare
@kfox1111 DCO has been added. |
079304f
to
b4829a7
Compare
b4829a7
to
ca83e3f
Compare
7689432
to
593c46a
Compare
593c46a
to
cd225fd
Compare
4c4ff06
to
8b8954a
Compare
…5395) Signed-off-by: Dmitry Kartsev <[email protected]>
88502de
to
1d56332
Compare
@sorindumitru if everything in place, please approve. |
As a part of #5395 this PR add support to Minio S3 storage.
The PR does not meant to be merge to main yet.
I do need someone look on the PR and provide initial feedback.
I wanna make sure this functionality is still needed and overall logic does make sense.
if all good, I'll continue to work on it, add tests, etc...