quarkiverse / quarkus-minio

Minio (https://min.io) Client Quarkus Extension
Apache License 2.0
18 stars 24 forks source link

Allow configless programmatic only creation of MinioClient #178

Closed ppalaga closed 1 year ago

ppalaga commented 1 year ago

Before https://github.com/quarkiverse/quarkus-minio/pull/166 the named scenario was possible and we have been relying on it in Camel Quarkus.

After https://github.com/quarkiverse/quarkus-minio/pull/166 it does not work anymore

Could you please make config of the default client optional so that we can create the client 100% programmatically without any config in application.properties?

ppalaga commented 1 year ago

BTW, what we ask for is a common principle accepted in many Quarkiverse extensions.

jtama commented 1 year ago

Have you tried : quarkus.minio.enabled=false ?

We may need more documentation here.. If it works, I willl add the related documentation and close the issue.

ppalaga commented 1 year ago

Let me explain the situation in Camel Quarkus in more detail. We have a Quarkus extension called camel-quarkus-minio wrapping the minio client in a uniform Camel API. The client is configured in the way usual in Camel - i.e. through Camel specific properties or some DSL. We prefer not to advise our users to use any thrird party ways of configuration. To make this possible, we control the lifecycle of the minio client and also the ways how it is configured. camel-quarkus-minio leverages quarkus-minio, esp. for getting the native configuration.

Have you tried : quarkus.minio.enabled=false

I am not sure that would solve our problem. I guess quarkus.minio.enabled=false would typically have to be passed via application.properties by our users, which we'd like to avoid.

I see that there are ways to pass a config value programmatically (e.g. via io.quarkus.deployment.builditem.SystemPropertyBuildItem), but that's also not something we'd like to do, because we do not want to interfere with our user's preferences. They may want to use both Camel way of configuring the client as well as the quarkus-minio way of configuring the client.

We'd like to get into a state where providing no value for quarkus.minio.url would cause no error, and no client would be created by quarkus-minio. This could be done either by

A. changing the current programming logic, or

B. maybe by splitting the extension into 1. core part containing the native config and the devservice and 2. the client part containing everything else. With this solution in place, Camel Quarkus would depend only on the core part. End users wanting the quarkus-minio way of config would have to depend on the client part. The client part could be called quarkus-minio so there would be no change for quarkus-minio users.

Any opinion on that?

jtama commented 1 year ago

I have to chew on that and check if I can get back to the former behaviour withou having to split all (that would take more time I think).

ppalaga commented 1 year ago

BTW, if B. should become the prefered solution, I could prepare a PR.

jtama commented 1 year ago

Well B would be a problem as devservices needs to be aware of the needed minioClient... So the solution would be to only leave native cooking in the core... Unless I miss something...

jtama commented 1 year ago

Just for me to be sure. Before this change the property quarkus.minio.allow-empty had to be set to true, to enable your case (which are perfectly get). How did you do that before ?

If i re-add the allow-empty (which would only be activated if no other config is set), would it be good for you ?

There we have to choice :

ppalaga commented 1 year ago

Before this change the property quarkus.minio.allow-empty had to be set to true, to enable your case (which are perfectly get). How did you do that before ?

Indeed, we have been passing RunTimeConfigurationDefaultBuildItem("quarkus.minio.allow-empty", "true") programmatically.

If i re-add the allow-empty (which would only be activated if no other config is set

"would only be activated" - you mean the need for quarkus.minio.allow-empty=true could be autodetected by quarkus-minio?

would it be good for you?

Having quarkus.minio.allow-empty=true working the way it did before would definitely be better than the current state and we could live with it.

But still, passing quarkus.minio.allow-empty=true from our extension may clash with end user expectations about how much safety they have about not forgetting to set quarkus.minio.url. I'd personally find splitting into core and client a bit cleaner. But of course the decision is up to you.

jtama commented 1 year ago

I definitely like the second option best, but in this case it would be more like quarkus-minio-native...

jtama commented 1 year ago

I have put up a PR coulld you try this and see if it suits you ?

ppalaga commented 1 year ago

I have put up a PR coulld you try this and see if it suits you ?

It works great, thanks a lot!

When can we expect a release?

jtama commented 1 year ago

Sometime today 👍.

jtama commented 1 year ago

@ppalaga It's released!

ppalaga commented 1 year ago

Thanks!