Switch from localstack to minIO #2640
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on replacing the Changes
Poem
📝 Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
docker/minio/entrypoint.sh (1)
13-13: Wouldn't it be nice to know if the initialization actually worked?The initialization script runs silently, which is... interesting. Consider capturing and logging its output:
-sh /init-script.sh +if ! sh /init-script.sh > /tmp/init.log 2>&1; then + echo "Initialization failed. Check /tmp/init.log for details. Not that anyone reads logs anyway." + exit 1 +fidocker/.local.env (1)
15-16: Interesting choice of endpointsThe endpoints are configured for HTTP. While this works, it might be worth considering HTTPS for production environments. You know, just in case someone cares about security.
Consider adding:
BUCKET_USE_SSL=true # For production environmentsdocker-compose.yaml (2)
34-36: About those volume mounts...The data directory is mounted directly from the project path. I'm sure there's a good reason for this, but it might be worth considering using a named volume for better data persistence.
- - "./care/media/minio:/data" + - "minio-data:/data"Don't forget to add it to the volumes section:
volumes: minio-data:
41-46: The health check looks... optimisticWhile the health check is present (which is great!), we might want to add some resource limits to prevent our MinIO container from consuming all available resources. You know, just in case.
Add deploy limits:
deploy: resources: limits: memory: 2G reservations: memory: 1Gdocker/minio/init-script.sh (1)
29-39: Error handling could use a bit more... handlingThe bucket creation function assumes the command will succeed after passing the existence check. It would be wonderful if we could add proper error handling.
create_bucket_if_not_exists() { BUCKET_NAME=$1 echo "Checking if bucket $BUCKET_NAME exists..." if mc ls local/$BUCKET_NAME > /dev/null 2>&1; then echo "Bucket $BUCKET_NAME already exists. Skipping creation." else echo "Creating bucket $BUCKET_NAME..." - mc mb local/$BUCKET_NAME + if ! mc mb local/$BUCKET_NAME; then + echo "Failed to create bucket $BUCKET_NAME" + return 1 + fi fi }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
docker-compose.yaml(1 hunks)docker/.local.env(1 hunks)docker/.prebuilt.env(1 hunks)docker/awslocal/bucket-setup.sh(0 hunks)docker/minio/entrypoint.sh(1 hunks)docker/minio/init-script.sh(1 hunks)docs/local-setup/configuration.rst(1 hunks)
💤 Files with no reviewable changes (1)
- docker/awslocal/bucket-setup.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
docker/minio/init-script.sh
[warning] 17-17: In POSIX sh, 'local' is undefined.
(SC3043)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
docker/minio/entrypoint.sh (2)
3-4: Those hardcoded ports look... confidentWhile I'm sure you've thoroughly tested that ports 9001 will always be available, it might be slightly more robust to use environment variables.
-minio server /data --console-address ":9001" & +minio server /data --console-address ":${MINIO_CONSOLE_PORT:-9001}" &
23-24: Signal handling? Never heard of herConsider adding signal handling to ensure proper cleanup when the container is stopped.
+# Setup signal handlers +trap 'echo "Gracefully shutting down..."; kill $!; exit 0' SIGTERM SIGINT + # Keep the container running wait $!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
docker-compose.yaml(1 hunks)docker/.local.env(1 hunks)docker/.prebuilt.env(1 hunks)docker/minio/entrypoint.sh(1 hunks)docker/minio/init-script.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docker/.prebuilt.env
- docker/.local.env
- docker/minio/init-script.sh
🔇 Additional comments (3)
docker/minio/entrypoint.sh (1)
6-18: Well, well, look who implemented a proper timeout
The health check implementation with a 5-minute timeout is quite reasonable. Nice work on the error handling too.
docker-compose.yaml (2)
30-32: Environment variables for credentials - how thoughtful
Nice work using environment variables with fallback values for the credentials.
41-46: A properly configured healthcheck - what a pleasant surprise
The healthcheck configuration looks well-thought-out with appropriate intervals, timeouts, and retries.
|
@vigneshhari Please review. Would like to add that minIO doesn't seem to support "public-read" ACL set during upload, hence 'facility-bucket' is initialized as public. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2640 +/- ##
========================================
Coverage 69.63% 69.63%
========================================
Files 211 211
Lines 11879 11879
Branches 1202 1202
========================================
Hits 8272 8272
Misses 3240 3240
Partials 367 367 ☔ View full report in Codecov by Sentry. |
Proposed Changes
Associated Issue
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores