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

Broken AddMinio(...) method: Endpoint turns back to default #873

Closed
picolino opened this issue Sep 30, 2023 · 6 comments
Closed

Broken AddMinio(...) method: Endpoint turns back to default #873

picolino opened this issue Sep 30, 2023 · 6 comments
Assignees

Comments

@picolino
Copy link

picolino commented Sep 30, 2023

I connect Minio sdk in my services this way:

services.AddMinio(o =>
            {
                o.WithEndpoint("my.s3.com")
                    .WithCredentials(
                        Environment.GetEnvironmentVariable(s3AccessKeyEnv),
                        Environment.GetEnvironmentVariable(s3SecretKeyEnv)
                    );
            },
            ServiceLifetime.Scoped
        );

But when I try to use API through IMinioClient I have 'play.min.io' as an endpoint.

A little dig inside shows that in this code line we should also pass configureClient.
https://github.com/minio/minio-dotnet/blob/master/Minio/ServiceCollectionExtensions.cs#L47

And here Endpoint goes back to 'play.min.io':
https://github.com/minio/minio-dotnet/blob/master/Minio/MinioClientFactory.cs#L45

UPD: Minio version: 6.0.0

@picolino
Copy link
Author

picolino commented Sep 30, 2023

Workaround is pretty simple, we need to call Build() inside action, like that:

services.AddMinio(o =>
            {
                o.WithEndpoint("my.s3.com")
                    .WithCredentials(
                        Environment.GetEnvironmentVariable(s3AccessKeyEnv),
                        Environment.GetEnvironmentVariable(s3SecretKeyEnv)
                    )
                    .Build(); // Add this line to workaround this issue
            },
            ServiceLifetime.Scoped
        );

@picolino picolino changed the title Broken AddMinio(...) method Broken AddMinio(...) method: Endpoint turns back to default Sep 30, 2023
@ebozduman ebozduman self-assigned this Oct 4, 2023
@ebozduman
Copy link
Collaborator

@picolino ,

Calling .Build() is not a workaround here, it is a requirement. That is, whenever you define arguments to be fed to its function call, the .Withxxxx() calls are required to be followed by the .Build() function call at the end as the last call to manipulate and process the arguments data.
So, it works as expected, hence there is NO issue here.
Closed.

@Lightfire228
Copy link

If this works as expected, then you might want to update the documentation

per the Readme

        // Add Minio using the custom endpoint and configure additional settings for default MinioClient initialization
        builder.Services.AddMinio(configureClient => configureClient
            .WithEndpoint(endpoint)
            .WithCredentials(accessKey, secretKey));

@completej
Copy link

@picolino ,

Calling .Build() is not a workaround here, it is a requirement. That is, whenever you define arguments to be fed to its function call, the .Withxxxx() calls are required to be followed by the .Build() function call at the end as the last call to manipulate and process the arguments data. So, it works as expected, hence there is NO issue here. Closed.

One caveat to this. The .WithCredentials(accessKey, secretKey)) actually works without calling .Build(), which might have confused the issue. When refactoring this out to a separate class for keeping Program clean, another example could be

        // Add Minio using the custom endpoint and configure additional settings for default MinioClient initialization
        services.AddMinio(configureClient => configureClient
            .WithEndpoint(config["APIs:Minio:Endpoint"])
            .WithCredentials(config["APIs:Minio:AccessKey"], config["APIs:Minio:SecretKey"])
            .WithSSL(true)
            .Build());

@ebozduman
Copy link
Collaborator

@completej,

Good point!
We'll look into it.
Thank you for the follow up.

@ebozduman
Copy link
Collaborator

Clarification: Although Build() is required and it is a documentation issue, there is still a bug and inconsistencies among the 3 overloads of WithEndpoint() method.
The PR#844 addresses these issues, and it is currently in the code review process. It'll be merged soon.
This issue still can stay closed though as it is also a duplicate of issue #887 which will be held open to track the issue and the verification of its fix.

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

No branches or pull requests

4 participants