Closed chandel-aman closed 1 month ago
The changes integrate MinIO, an object storage service, into the project. This includes the introduction of an asynchronous function for configuring MinIO settings based on user input, enhancements to npm scripts for checking and running MinIO, and updates to the environment configurations. Additionally, the setup process is improved by prompting users for necessary credentials and ensuring the environment is correctly set up for MinIO usage.
Files | Change Summary |
---|---|
.eslintrc.json , setup.ts , package.json |
Updated ESLint configuration to include setup.ts , added a new asynchronous function configureMinio for configuring MinIO settings, and introduced new npm scripts for MinIO integration. |
Objective | Addressed | Explanation |
---|---|---|
Docker Integration for MinIO ( #2419 ) | ❓ | The PR does not explicitly detail Docker configurations. |
npm Script Integration for starting MinIO with API server ( #2419 ) | ✅ | |
Update project documentation for MinIO usage ( #2419 ) | ❌ | Documentation updates are not included in the PR. |
Ensure MinIO installation checks ( #2419 ) | ✅ |
The changes address some objectives from the linked issue, specifically the npm script integration and installation checks. However, Docker integration details and documentation updates are not fully addressed.
Possibly related PRs:
setup.ts
file, focusing on enhancing user prompts, which is related to the overall user interaction improvements in the main PR's new configureMinio
function.Suggested reviewers:
🐇 In fields of green, a treasure we find,
MinIO joins us, storage well-defined.
Docker and scripts make setup a breeze,
Hopping along with data's new ease!
So let’s celebrate with a joyful cheer,
Our files secure, with MinIO near! 🥕✨
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
We have these basic policies to make the approval process smoother for our volunteer team.
Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.
Do not assign reviewers. Our Queue Monitors will review your PR and assign them. When your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Your reviewer(s) will have the following roles:
Read our CONTRIBUTING.md file. Most importantly:
Attention: Patch coverage is 73.09417%
with 60 lines
in your changes missing coverage. Please review.
Project coverage is 98.77%. Comparing base (
c952c7a
) to head (c177553
). Report is 1 commits behind head on develop.
Files with missing lines | Patch % | Lines |
---|---|---|
src/minioInstallationCheck.ts | 0.00% | 60 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@palisadoes @aashimawadhwa Could you please review this PR?
@aashimawadhwa @tasneemkoushar PTAL
Please also work on the code rabbit suggestions for improving the code base
@chandel-aman did you make any fix for the error that @palisadoes pointed to? I tried doing the setup but didn't get that error in my case
@palisadoes works fine with sample imports as well
@chandel-aman while docker setup step, i got this error response, is this anyhow related to your change
@chandel-aman Tested minio local setup, works fine
I get this error when trying it.
I was evaluating a PR before that made some DB updates as part of the setup process. This may have been the cause.
- Make sure that the setup script drops the entire database before loading data in both the default and sample data loading steps.
These were the values I used in the setup. You can see whether this was the cause too.
MINIO_ENDPOINT=http://localhost:9000 MINIO_ROOT_USER=peter MINIO_ROOT_PASSWORD=peter-talawa MINIO_BUCKET=peter-bucket
The error you're experiencing is not related to the Minio setup or the environment variables you shared. It's a MongoDB duplicate key error in the talawa.api.users
collection.
The error occurs because of duplicate null values in the "identifier" field, which has a unique index. This is likely due to the previous database state rather than the Minio configuration.
I've made changes to drop the database before loading data in both default and sample data loading steps.
The setup script doesn't check to see whether MinIO is installed. I ran the setup script successfully using the default data and there was no error.
Please make sure the appropriate checks are made
Please work with you mentors on the docker setup verification
Yes, you're correct that the setup script currently does not check for MinIO installation.
Instead, the check and installation are performed when the user runs the command npm run <dev or start>:with-minio
. Would you like me to include the check and installation for MinIO in the initial setup script itself?
@chandel-aman including a check would work and can you please check the lining or some formatting issue in your commit.
@chandel-aman did you make any fix for the error that @palisadoes pointed to? I tried doing the setup but didn't get that error in my case
No, I did not make any fixes for the error pointed by @palisadoes. The error is not caused by the changes in this PR but is likely due to existing changes made to the database.
@chandel-aman while docker setup step, i got this error response, is this anyhow related to your change
No, this error is not related to my changes. It might be occurring due to this line: ${PWD}/Caddyfile:/etc/caddy/Caddyfile
.
It could be either a path error or a permission error.
I've verified that the Docker setup is running correctly.
@chandel-aman including a check would work and can you please check the lining or some formatting issue in your commit.
Sure, I'll make the changes to include a check for Minio in the setup.
I’ve fixed the linting error; however, other linting issues are occurring due to the @typescript-eslint/ban-types rule, which has been deprecated in version 8. You can find more information here. This might be related to the PR #2485.
I’ve fixed the linting error; however, other linting issues are occurring due to the @typescript-eslint/ban-types rule, which has been deprecated in version 8. You can find more information here. This might be related to the PR #2485.
Please create an issue to fix this so that we have better code quality. Hopefully someone else will give it a try.
@chandel-aman do let me know when you PR is ready for merging
data/
directory? This should be documented and possibly given as a setup option.data/
directory, is the images/
directory needed in the repo?data/
directory is required, then it should be added to the repo. Take a look at what was done for the images/
directory, both in its content and its treatment in the .gitignore
filePlease make the coderabbit.ai edits
Please make sure that the instructions in INSTALLATION.md are clear and flow logically. For example, we now have a new prerequisite to make the application work.
Also, how will the user be able to change the location of the
data/
directory? This should be documented and possibly given as a setup option.If all objects are going to be uploaded to the
data/
directory, is theimages/
directory needed in the repo?If the
data/
directory is required, then it should be added to the repo. Take a look at what was done for theimages/
directory, both in its content and its treatment in the.gitignore
file
data/
directory configurable so as to allow user to change the location of this directory.images/
or videos/
directory, and hence we can remove them from the repo.data/
directory is not required to be present in the repo. I did look into the earlier configuration, we are not including any directories which stored files.@chandel-aman do let me know when you PR is ready for merging
@tasneemkoushar @aashimawadhwa Could you please review this PR?
Hey @chandel-aman this looks good to me i have tried running this on my local is working good @palisadoes we can merge it, it's gtg.
@palisadoes @tasneemkoushar Could you please provide your review?
Please fix the failing tests
Please fix the failing tests
The failing test has been fixed. Could you please merge this PR?
@palisadoes this is gtg
What kind of change does this PR introduce?
Feature: Adds MinIO setup and configuration instructions for both local and Docker environments.
Issue Number:
Fixes #2419
Did you add tests for your changes?
Yes
Snapshots/Videos:
Added MinIO setup
https://github.com/user-attachments/assets/ff8783ae-9053-4732-8cea-1b9b4c8cfe5c
MinIO docker setup
https://github.com/user-attachments/assets/0007cad5-014b-48e0-aa3e-818185852f0c
MinIO local setup
https://github.com/user-attachments/assets/0fe0f143-b8bd-4b7e-9db3-4855a5ff0e6e
If relevant, did you update the documentation?
Yes
Summary
This PR introduces MinIO setup instructions into the project to streamline its deployment and configuration. Specifically, it adds:
.gitignore
,.dockerignore
, andpackage.json
to exclude MinIO metadata and update relevant scripts.create_env.py
andenvSchema
to include MinIO-specific environment variables.installation.md
for setting up MinIO locally and with Docker.Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor