feat(minio): ensure bucket exists on storage init#43
Open
oismaelash wants to merge 2 commits intoEvolutionAPI:mainfrom
Open
feat(minio): ensure bucket exists on storage init#43oismaelash wants to merge 2 commits intoEvolutionAPI:mainfrom
oismaelash wants to merge 2 commits intoEvolutionAPI:mainfrom
Conversation
Create the configured bucket when missing, matching fix/create-bucket-if-no-exist-minio. Add unit tests (fake client) and optional integration test behind -tags=integration. Made-with: Cursor
Reviewer's GuideThis PR updates MinIO-backed media storage initialization to ensure the configured bucket exists at startup, introducing a small abstraction over the MinIO client plus unit and optional integration tests for bucket creation behavior. Sequence diagram for MinIO media storage initialization with bucket creationsequenceDiagram
actor Application
participant NewMinioMediaStorage
participant MinioClient as minio.Client
Application->>NewMinioMediaStorage: initialize(bucketName, region, otherParams)
NewMinioMediaStorage->>MinioClient: minio.New(endpoint, credentials, useSSL)
MinioClient-->>NewMinioMediaStorage: client or error
alt client_creation_error
NewMinioMediaStorage-->>Application: error
else client_created
NewMinioMediaStorage->>MinioClient: BucketExists(ctx, bucketName)
MinioClient-->>NewMinioMediaStorage: exists, error
alt bucket_exists_error
NewMinioMediaStorage-->>Application: error
else bucket_missing
NewMinioMediaStorage->>MinioClient: MakeBucket(ctx, bucketName, MakeBucketOptions{Region: region})
MinioClient-->>NewMinioMediaStorage: error or success
alt bucket_creation_error
NewMinioMediaStorage-->>Application: error
else bucket_created
NewMinioMediaStorage->>MinioClient: setBucketPolicy(client, bucketName)
MinioClient-->>NewMinioMediaStorage: error or success
NewMinioMediaStorage-->>Application: *MinioMediaStorage or error
end
else bucket_already_exists
NewMinioMediaStorage->>MinioClient: setBucketPolicy(client, bucketName)
MinioClient-->>NewMinioMediaStorage: error or success
NewMinioMediaStorage-->>Application: *MinioMediaStorage or error
end
end
Class diagram for MinIO media storage and bucket admin abstractionclassDiagram
class minioBucketAdmin {
<<interface>>
+BucketExists(ctx context.Context, bucketName string) bool, error
+MakeBucket(ctx context.Context, bucketName string, opts minio.MakeBucketOptions) error
}
class MinioClient {
<<struct from minio.Client>>
}
class MinioMediaStorage {
-client *minio.Client
-bucketName string
-baseURL string
-useSSL bool
+NewMinioMediaStorage(endpoint string, accessKeyID string, secretAccessKey string, useSSL bool, bucketName string, region string, baseURL string) *MinioMediaStorage, error
+Upload(ctx context.Context, objectName string, reader io.Reader, size int64, contentType string) (string, error)
+Delete(ctx context.Context, objectName string) error
+GetURL(objectName string) string
}
class ensureBucketExists {
+ensureBucketExists(ctx context.Context, client minioBucketAdmin, bucketName string, region string) error
}
MinioClient ..|> minioBucketAdmin
MinioMediaStorage o--> MinioClient : uses
ensureBucketExists ..> minioBucketAdmin : depends_on
MinioMediaStorage ..> ensureBucketExists : calls_during_init
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Avoid using fmt.Printf in ensureBucketExists; either remove the side-effectful log or route it through the existing logging/observability mechanism used elsewhere in the package.
- Consider accepting a context parameter for NewMinioMediaStorage (and passing it into ensureBucketExists) instead of using context.Background(), so callers can control cancellation/timeouts for the potentially slow bucket-existence and creation operations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid using fmt.Printf in ensureBucketExists; either remove the side-effectful log or route it through the existing logging/observability mechanism used elsewhere in the package.
- Consider accepting a context parameter for NewMinioMediaStorage (and passing it into ensureBucketExists) instead of using context.Background(), so callers can control cancellation/timeouts for the potentially slow bucket-existence and creation operations.
## Individual Comments
### Comment 1
<location path="pkg/storage/minio/media_storage.go" line_range="28-34" />
<code_context>
+ if err != nil {
+ return fmt.Errorf("failed to create bucket: %w", err)
+ }
+ fmt.Printf("Successfully created bucket %s\n", bucketName)
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Avoid direct fmt.Printf in library code and rely on a logger or remove non-essential logging.
Writing to stdout from this helper can be unexpected for callers and noisy in production. Please either remove this success log or send it through the service’s logger so callers can control log levels and outputs.
```suggestion
if !exists {
err = client.MakeBucket(ctx, bucketName, minio.MakeBucketOptions{Region: region})
if err != nil {
return fmt.Errorf("failed to create bucket: %w", err)
}
}
```
</issue_to_address>
### Comment 2
<location path="pkg/storage/minio/media_storage_integration_test.go" line_range="22" />
<code_context>
+func TestNewMinioMediaStorage_CreatesBucket(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Integration test name suggests bucket creation but the test only asserts successful initialization
Right now it only verifies that `NewMinioMediaStorage` returns a non-nil storage without error. Please either extend it to assert that the bucket actually exists after initialization (e.g. via a MinIO client and `BucketExists` on the configured bucket), or rename the test (for example to `TestNewMinioMediaStorage_InitializesStorage`) so the behavior is accurately described.
```suggestion
func TestNewMinioMediaStorage_InitializesStorage(t *testing.T) {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+28
to
+34
| if !exists { | ||
| err = client.MakeBucket(ctx, bucketName, minio.MakeBucketOptions{Region: region}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create bucket: %w", err) | ||
| } | ||
| fmt.Printf("Successfully created bucket %s\n", bucketName) | ||
| } |
There was a problem hiding this comment.
suggestion: Avoid direct fmt.Printf in library code and rely on a logger or remove non-essential logging.
Writing to stdout from this helper can be unexpected for callers and noisy in production. Please either remove this success log or send it through the service’s logger so callers can control log levels and outputs.
Suggested change
| if !exists { | |
| err = client.MakeBucket(ctx, bucketName, minio.MakeBucketOptions{Region: region}) | |
| if err != nil { | |
| return fmt.Errorf("failed to create bucket: %w", err) | |
| } | |
| fmt.Printf("Successfully created bucket %s\n", bucketName) | |
| } | |
| if !exists { | |
| err = client.MakeBucket(ctx, bucketName, minio.MakeBucketOptions{Region: region}) | |
| if err != nil { | |
| return fmt.Errorf("failed to create bucket: %w", err) | |
| } | |
| } |
| // MINIO_REGION=us-east-1 \ | ||
| // MINIO_USE_SSL=false \ | ||
| // go test -tags=integration ./pkg/storage/minio/... | ||
| func TestNewMinioMediaStorage_CreatesBucket(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Integration test name suggests bucket creation but the test only asserts successful initialization
Right now it only verifies that NewMinioMediaStorage returns a non-nil storage without error. Please either extend it to assert that the bucket actually exists after initialization (e.g. via a MinIO client and BucketExists on the configured bucket), or rename the test (for example to TestNewMinioMediaStorage_InitializesStorage) so the behavior is accurately described.
Suggested change
| func TestNewMinioMediaStorage_CreatesBucket(t *testing.T) { | |
| func TestNewMinioMediaStorage_InitializesStorage(t *testing.T) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Create the configured bucket when missing, matching fix/create-bucket-if-no-exist-minio. Add unit tests (fake client) and optional integration test behind -tags=integration.
Made-with: Cursor
Description
Related Issue
Closes #(issue_number)
Type of Change
Testing
Screenshots (if applicable)
Checklist
Additional Notes
Summary by Sourcery
Ensure the configured MinIO bucket is created during media storage initialization and add coverage for this behavior.
New Features:
Tests: