- 
                Notifications
    You must be signed in to change notification settings 
- Fork 132
Add Minio hosting/client integration #691
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
Add Minio hosting/client integration #691
Conversation
bced977    to
    6d8e405      
    Compare
  
    | Would love to see this get finished off. | 
6d8e405    to
    1a79db3      
    Compare
  
            
          
                src/CommunityToolkit.Aspire.Hosting.Minio/MinioBuilderExtensions.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | IResourceBuilder<ParameterResource>? rootUser = null, | ||
| IResourceBuilder<ParameterResource>? rootPassword = null, | ||
| int minioPort = 9000, | ||
| int minioConsolePort = 9001) | 
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.
Remove, this is usually fixed
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.
Thanks!
This is usually fixed, and thinking too much about Minio Console should probably be out of scope for now.
|  | ||
| namespace CommunityToolkit.Aspire.Hosting.Minio; | ||
|  | ||
| internal sealed class MinioHealthCheck : IHealthCheck | 
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.
You might be able to use the built in http health check instead of the custom one.
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.
Built in http health check using .AddUrlGroup method really looks better
Thx for comment
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.
Well there’s an even more built in method WithHttpHealthCheck
| This looks really good! Left a few comments. | 
| I've added WithDataVolume/WithDataBindMount methods to this, because even a basic S3-compatible storage integration wihtout the way to permanently store data isn't up to my personal quality standarts Also working on @davidfowl comments got me thinking that me "inventing" a connection string for MiniO looking like may not have been such a good idea (here's a link for further reference) But currently I cannot think about any other convinient way of passing connection parameters to ApiServices in a DistributedApplication. I'd be genuinely glad to hear any opinions on this | 
        
          
                src/CommunityToolkit.Aspire.Hosting.Minio/MinioContainerResource.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | 
 It's a good idea. Just expose all of the parts of the connection string so the users can make their own connection strings, but having a default is good. | 
        
          
                tests/CommunityToolkit.Aspire.Testing/TestDistributedApplicationBuilder.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | I've fixed my silly little tests that were conflicting with port randomization Now it looks ready | 
ce4533d    to
    b23e8b2      
    Compare
  
    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.
Looks pretty good. Got a few changes I've noted.
Also, the MinIO docs have the IO part in all capitals (as does the NuGet package), but there's inconsistency in the readme/doc comments - can we have that updated to match the product naming please.
        
          
                src/CommunityToolkit.Aspire.Hosting.Minio/CommunityToolkit.Aspire.Hosting.Minio.csproj
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/CommunityToolkit.Aspire.Hosting.Minio/CommunityToolkit.Aspire.Hosting.Minio.csproj
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/CommunityToolkit.Aspire.Hosting.Minio/MinioBuilderExtensions.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | public MinioContainerResource(string name, ParameterResource user, ParameterResource password) : base(name) | ||
| { | ||
| RootUser = user; | ||
| PasswordParameter = password; | ||
| } | 
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.
Can you use a primary constructor for this
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.
Yes, it looks much cleaner now, thanks!
        
          
                src/CommunityToolkit.Aspire.Minio.Client/MinioClientBuilderExtensionMethods.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/CommunityToolkit.Aspire.Minio.Client/MinioClientBuilderExtensionMethods.cs
          
            Show resolved
            Hide resolved
        
      | Assert.Equal(HttpStatusCode.OK, createResponse.StatusCode); | ||
|  | ||
| var getResponse = await httpClient.GetAsync($"/buckets/{bucketName}"); | ||
| Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); | 
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.
Can we assert on the returned value, not just the status code?
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.
I've expanded the test a bit, so we can actually upload a string object, then download it and check the result
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.
@Harold-Morgan - I noticed you've added yourself to the CODEOWNERS - are you happy to be added as a contributor to the repo as the long-term owner of the integration?
| I would definitely be more than happy to be added as a long-term owner of the integration. I was planning on adding features and solving issues in the future myself, it is a wonderful opportunity to learn! @aaronpowell I must note that I haven't yet addressed all the comments in this review (but definitely plan to do so), but you have approved it. Is this ok? I think that I definitely must address all of them in this PR before merging | 
| Sorry, that was a mistake on the approval - notifications took me to only seeing some of the changes and I forgot I had more on the list outstanding so I've rolled it back to waiting approval. | 
| When it can be available from NuGet package? | 
Co-authored-by: Aaron Powell <me@aaron-powell.com>
6fd3a24    to
    90a326e      
    Compare
  
    | 
 A preview package should be on NuGet soon (takes a few days to go through approval), but you can always use the Azure DevOps feed as described on the README | 
| @Harold-Morgan - thanks for doing this!! | 
| Really high quality work @Harold-Morgan ! | 
| 
 Jokes aside, sincerely thank you and @aaronpowell for help! I hope and plan to do more | 

**Closes #606 **
I've added maybe somewhat barebone (but working!) MiniO storage and client (based on the official https://github.com/minio/minio-dotnet library). It was a ton of fun.
I've added
to ConformanceTests.cs/ConnectionInformationIsDelayValidated because this integration doesn't support keyed registrations untill minio/minio-dotnet#1147 is resolved. I hope this isn't too much, but maybe it should've been done in separate PR.
PR Checklist
Other information