-
Notifications
You must be signed in to change notification settings - Fork 2
feat: generate a minio client #201
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
Conversation
Reviewer's Guide by SourceryThis pull request implements the generation of a Minio client for S3-compatible storage in the SettleMint SDK. It adds support for Minio configuration in the SDK generation process and includes necessary changes to integrate Minio functionality into the existing codebase. Sequence DiagramsequenceDiagram
participant User
participant CodegenCommand
participant SDKGenerator
participant MinioClient
User->>CodegenCommand: Run codegen command
CodegenCommand->>SDKGenerator: Generate SDK
alt Minio configuration exists
SDKGenerator->>MinioClient: Create Minio S3 client
MinioClient-->>SDKGenerator: Return Minio client
end
SDKGenerator-->>CodegenCommand: Return generated SDK
CodegenCommand-->>User: SDK generated successfully
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @roderik - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider making the Minio client port and SSL usage configurable instead of hardcoding them.
- It might be beneficial to document the new SETTLEMINT_MINIO_ACCESS_KEY and SETTLEMINT_MINIO_SECRET_KEY environment variables, and possibly explore more secure ways of handling these credentials.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Review instructions: 2 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,11 @@ | |||
| import { Client } from "minio"; | |||
|
|
|||
| export function createMinioS3Client({ endPoint }: { endPoint: string }) { | |||
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.
suggestion (review_instructions): Consider adding error handling for potential connection issues.
While the code is functional, it might be beneficial to add error handling to manage potential connection failures or invalid credentials. This would improve the robustness of the function.
export function createMinioS3Client({ endPoint }: { endPoint: string }): Promise<Client> {
return new Promise((resolve, reject) => {
try {
const client = new Client({
endPoint: endPoint,
// ... other configuration options
});
resolve(client);
} catch (error) {
reject(new Error(`Failed to create Minio S3 client: ${error.message}`));
}
});
}
Review instructions:
Path patterns: **/*.ts
Instructions:
- You always use the latest version of NodeJS, Bun, React, NestJS and NextJS, and you are familiar with the latest features and best practices.
- Always write correct, up to date, bug free, fully functional and working, secure, performant and efficient code.
- Focus on readability over being performant, but performance is important.
| accessKey: process.env.SETTLEMINT_MINIO_ACCESS_KEY ?? "", | ||
| secretKey: process.env.SETTLEMINT_MINIO_SECRET_KEY ?? "", |
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.
suggestion (review_instructions): Consider adding validation for required environment variables.
It's a good practice to validate that these environment variables are set before using them. If they're required for the client to function correctly, you might want to throw an error if they're not present.
const accessKey = process.env.SETTLEMINT_MINIO_ACCESS_KEY;
const secretKey = process.env.SETTLEMINT_MINIO_SECRET_KEY;
if (!accessKey || !secretKey) {
throw new Error('Minio access key and secret key are required');
}
accessKey: accessKey,
secretKey: secretKey,
Review instructions:
Path patterns: **/*.ts
Instructions:
- You always use the latest version of NodeJS, Bun, React, NestJS and NextJS, and you are familiar with the latest features and best practices.
- Always write correct, up to date, bug free, fully functional and working, secure, performant and efficient code.
- Focus on readability over being performant, but performance is important.
| port: 443, | ||
| useSSL: true, |
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.
suggestion: Make port and SSL settings configurable
The hardcoded port and SSL settings might not be flexible enough for all use cases. Consider making these configurable by accepting them as parameters or reading from configuration.
export function createMinioS3Client({ endPoint, port = 443, useSSL = true }: { endPoint: string; port?: number; useSSL?: boolean }) {
return new Client({
endPoint: endPoint,
port,
useSSL,
accessKey: process.env.SETTLEMINT_MINIO_ACCESS_KEY ?? "",
secretKey: process.env.SETTLEMINT_MINIO_SECRET_KEY ?? "",
📦 Packages
|
Summary by Sourcery
Add support for generating a MinIO S3 client in the SettleMint SDK, enabling integration with MinIO storage services. Update the code generation process to incorporate MinIO configuration when present.
New Features:
Enhancements: