-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add MinIO configuration #20
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Pratiksha Sankhe <[email protected]>
Reviewer's Guide by SourceryThis pull request adds MinIO configuration support to the cloud storage handler. It introduces new configuration variables, a MinioClient class for managing MinIO client setup and bucket creation, and updates the configuration model to include MinIO settings. The changes improve the modularity and maintainability of the code while providing necessary functionality for MinIO integration. File-Level Changes
Tips
|
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 @psankhe28 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 23 23
=========================================
Hits 23 23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
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 @psankhe28 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add comprehensive unit tests for the new MinIO functionality to ensure reliability and ease of future maintenance.
- Consider adding more detailed docstrings for public methods and classes, particularly in the new files like minio_client.py and custom_config.py.
- In the MinioConfig class, consider adding a warning log when falling back to default configurations to encourage users to provide their own settings.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟡 Security: 2 issues found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
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'm a bit confused. Started reviewing and found that some of the things are already addressed further down in some other/better way. Can you make sure the PR is cleaned up and only what needs to be in here (in the best possible way) is in here?
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
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 @psankhe28 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 2 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 3 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
"""Initialize MinioConfig with a configuration file. | ||
|
||
Args: | ||
config_file (str): Path to the configuration file. If not provided, |
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 datatype in all the pydocs, Ill leave it to you and wont comment again as its repetitive :).
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.
@psankhe28 you have resolved this conversation (mmaybe by mistake), but haven't remove d the datatype from pydocs?
# Current
config_file (str): Path to the configuration file.
# Change to
config_file : Path to the configuration file.
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.
Sorry @JaeAeich , I couldn't find this line: config_file (str): Path to the configuration file.
I have removed it already
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
@psankhe28 Can you please go through the previous conversation again (ping me here itself if something doesn't make sense), but I see there are alot of convos that have been closed but not resolved, it makes no sense for me to rewrite those review as they haven't been resolved. |
@JaeAeich , I think you have not seen the updated version. |
Description
Added configuration variables required for MinIO.
Fixes #18
Checklist
of this project, including, in particular, with regard to any style guidelines
Conventional Commits specification; in particular, it clearly
indicates that a change is a breaking change
using the PR title as the commit message
changed behavior
or updated existing ones (only for Python, TypeScript, etc.)
(Google-style Python docstrings) for all
packages/modules/functions/classes/methods or updated existing ones
works
Comments
Summary by Sourcery
Add support for MinIO configuration and client management, enabling the application to interact with MinIO storage. This includes defining configuration settings, creating a MinIO client, and handling bucket creation. Additionally, enhance the application with a custom configuration model and utility functions to manage configurations.
New Features:
Enhancements:
Documentation: