Switch from localstack to minIO #2640
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on replacing the Changes
Poem
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)
docker/minio/entrypoint.sh
Outdated
| until curl -s http://localhost:9000/minio/health/ready; do | ||
| echo "Waiting for MinIO to be ready..." | ||
| sleep 5 | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
I see we're feeling optimistic about the health check
While the health check is... functional, it might be nice to add a timeout to prevent infinite waiting. You know, just in case something goes terribly wrong.
Here's a slightly more robust version:
+TIMEOUT=300 # 5 minutes should be enough, right?
+start_time=$(date +%s)
until curl -s http://localhost:9000/minio/health/ready; do
+ current_time=$(date +%s)
+ elapsed=$((current_time - start_time))
+ if [ $elapsed -gt $TIMEOUT ]; then
+ echo "MinIO failed to start after ${TIMEOUT} seconds. But I'm sure you knew that could happen."
+ exit 1
+ fi
echo "Waiting for MinIO to be ready..."
sleep 5
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| until curl -s http://localhost:9000/minio/health/ready; do | |
| echo "Waiting for MinIO to be ready..." | |
| sleep 5 | |
| done | |
| TIMEOUT=300 # 5 minutes should be enough, right? | |
| start_time=$(date +%s) | |
| until curl -s http://localhost:9000/minio/health/ready; do | |
| current_time=$(date +%s) | |
| elapsed=$((current_time - start_time)) | |
| if [ $elapsed -gt $TIMEOUT ]; then | |
| echo "MinIO failed to start after ${TIMEOUT} seconds. But I'm sure you knew that could happen." | |
| exit 1 | |
| fi | |
| echo "Waiting for MinIO to be ready..." | |
| sleep 5 | |
| done |
docker/.local.env
Outdated
| BUCKET_KEY=minioadmin | ||
| BUCKET_SECRET=minioadmin |
There was a problem hiding this comment.
Oh, we're using default credentials. How... convenient.
Using minioadmin as both username and password might not be the most secure approach. I'm sure you have your reasons, but perhaps we could make it slightly less obvious?
Consider using environment-specific credentials:
-BUCKET_KEY=minioadmin
-BUCKET_SECRET=minioadmin
+BUCKET_KEY=${MINIO_ACCESS_KEY:-minioadmin}
+BUCKET_SECRET=${MINIO_SECRET_KEY:-minioadmin}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BUCKET_KEY=minioadmin | |
| BUCKET_SECRET=minioadmin | |
| BUCKET_KEY=${MINIO_ACCESS_KEY:-minioadmin} | |
| BUCKET_SECRET=${MINIO_SECRET_KEY:-minioadmin} |
docker-compose.yaml
Outdated
| MINIO_ROOT_USER: minioadmin | ||
| MINIO_ROOT_PASSWORD: minioadmin |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Credentials in plain text. Classic.
While having credentials directly in the compose file is... charming, it might be better to use environment variables.
- MINIO_ROOT_USER: minioadmin
- MINIO_ROOT_PASSWORD: minioadmin
+ MINIO_ROOT_USER: ${MINIO_ACCESS_KEY:-minioadmin}
+ MINIO_ROOT_PASSWORD: ${MINIO_SECRET_KEY:-minioadmin}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MINIO_ROOT_USER: minioadmin | |
| MINIO_ROOT_PASSWORD: minioadmin | |
| MINIO_ROOT_USER: ${MINIO_ACCESS_KEY:-minioadmin} | |
| MINIO_ROOT_PASSWORD: ${MINIO_SECRET_KEY:-minioadmin} |
docker/minio/init-script.sh
Outdated
| # Function to set a bucket public | ||
| set_bucket_public() { | ||
| BUCKET_NAME=$1 | ||
| echo "Setting bucket $BUCKET_NAME as public..." | ||
| mc anonymous set public local/$BUCKET_NAME | ||
| } |
There was a problem hiding this comment.
Public bucket access? How... brave of you
Setting a bucket to public access requires careful consideration of security implications. While I notice this aligns with the PR objective mentioning "facility bucket is converted to public access", we should probably add some warning comments for future maintainers.
# Function to set a bucket public
set_bucket_public() {
BUCKET_NAME=$1
+ # WARNING: This bucket is intentionally set to public access as MinIO doesn't support ACLs
+ # Ensure only non-sensitive data is stored in this bucket
echo "Setting bucket $BUCKET_NAME as public..."
mc anonymous set public local/$BUCKET_NAME
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Function to set a bucket public | |
| set_bucket_public() { | |
| BUCKET_NAME=$1 | |
| echo "Setting bucket $BUCKET_NAME as public..." | |
| mc anonymous set public local/$BUCKET_NAME | |
| } | |
| # Function to set a bucket public | |
| set_bucket_public() { | |
| BUCKET_NAME=$1 | |
| # WARNING: This bucket is intentionally set to public access as MinIO doesn't support ACLs | |
| # Ensure only non-sensitive data is stored in this bucket | |
| echo "Setting bucket $BUCKET_NAME as public..." | |
| mc anonymous set public local/$BUCKET_NAME | |
| } |
docker/minio/init-script.sh
Outdated
| retry_command() { | ||
| local cmd=$1 | ||
| until $cmd; do | ||
| RETRY_COUNT=$((RETRY_COUNT + 1)) | ||
| if [ $RETRY_COUNT -ge $MAX_RETRIES ]; then | ||
| echo "Command failed after $MAX_RETRIES attempts. Exiting..." | ||
| exit 1 | ||
| fi | ||
| echo "Command failed. Retrying ($RETRY_COUNT/$MAX_RETRIES)..." | ||
| sleep $RETRY_DELAY | ||
| done | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
That's a nice retry mechanism you've got there... would be even better with proper POSIX compliance
The retry logic is well-implemented, but the local keyword isn't POSIX-compliant. I'm sure you meant to use a more portable approach.
- local cmd=$1
+ cmd=$1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| retry_command() { | |
| local cmd=$1 | |
| until $cmd; do | |
| RETRY_COUNT=$((RETRY_COUNT + 1)) | |
| if [ $RETRY_COUNT -ge $MAX_RETRIES ]; then | |
| echo "Command failed after $MAX_RETRIES attempts. Exiting..." | |
| exit 1 | |
| fi | |
| echo "Command failed. Retrying ($RETRY_COUNT/$MAX_RETRIES)..." | |
| sleep $RETRY_DELAY | |
| done | |
| } | |
| retry_command() { | |
| cmd=$1 | |
| until $cmd; do | |
| RETRY_COUNT=$((RETRY_COUNT + 1)) | |
| if [ $RETRY_COUNT -ge $MAX_RETRIES ]; then | |
| echo "Command failed after $MAX_RETRIES attempts. Exiting..." | |
| exit 1 | |
| fi | |
| echo "Command failed. Retrying ($RETRY_COUNT/$MAX_RETRIES)..." | |
| sleep $RETRY_DELAY | |
| done | |
| } |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: In POSIX sh, 'local' is undefined.
(SC3043)
| BUCKET_KEY=minioadmin | ||
| BUCKET_SECRET=minioadmin |
There was a problem hiding this comment.
Default credentials in a prebuilt environment? How... convenient
Using default MinIO credentials (minioadmin) in a prebuilt environment might not be the best security practice. While it's technically functional, we should probably add a comment suggesting users to change these in production.
+# WARNING: These are default MinIO credentials. Ensure to change these in production environments
BUCKET_KEY=minioadmin
BUCKET_SECRET=minioadmin📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BUCKET_KEY=minioadmin | |
| BUCKET_SECRET=minioadmin | |
| # WARNING: These are default MinIO credentials. Ensure to change these in production environments | |
| BUCKET_KEY=minioadmin | |
| BUCKET_SECRET=minioadmin |
| @@ -16,7 +16,7 @@ Using Docker Compose | |||
| - care (main repo) | |||
| - redis (in-memory cache) | |||
| - celery (task queue) | |||
| - localstack (to mimic AWS services locally) | |||
| - minio (to mimic AWS services locally) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
The documentation seems to be missing some... details
While you've updated the service list to include MinIO, it would be absolutely wonderful if we could add some MinIO-specific setup instructions, such as:
- How to access the MinIO console (typically at http://localhost:9001)
- Default credentials for the MinIO console
- Any specific requirements or considerations for the MinIO setup
- minio (to mimic AWS services locally)
+ The MinIO console is available at http://localhost:9001
+ Default credentials: minioadmin/minioadmin
+ Note: The facility bucket is set to public access📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - minio (to mimic AWS services locally) | |
| - minio (to mimic AWS services locally) | |
| The MinIO console is available at http://localhost:9001 | |
| Default credentials: minioadmin/minioadmin | |
| Note: The facility bucket is set to public access |
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.
| # Run the bucket setup script | ||
| sh /init-script.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assuming init-script.sh will always be there, are we?
It might be worth checking if the initialization script exists and is executable before blindly running it.
# Run the bucket setup script
+if [ ! -f /init-script.sh ]; then
+ echo "Init script not found. I'm sure this is fine."
+ exit 1
+fi
+if [ ! -x /init-script.sh ]; then
+ echo "Init script not executable. How mysterious."
+ exit 1
+fi
sh /init-script.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run the bucket setup script | |
| sh /init-script.sh | |
| # Run the bucket setup script | |
| if [ ! -f /init-script.sh ]; then | |
| echo "Init script not found. I'm sure this is fine." | |
| exit 1 | |
| fi | |
| if [ ! -x /init-script.sh ]; then | |
| echo "Init script not executable. How mysterious." | |
| exit 1 | |
| fi | |
| sh /init-script.sh |
| - "./care/media/minio:/data" | ||
| - "./docker/minio/init-script.sh:/init-script.sh:ro" # Mount the init script | ||
| - "./docker/minio/entrypoint.sh:/entrypoint.sh:ro" # Mount the entrypoint script |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The mysterious case of the missing volume definition
While the volume mounts look correct, you might want to define a named volume for the MinIO data directory in the volumes section. You know, for persistence and all that.
volumes:
postgres-data:
redis-data:
+ minio-data:
services:
minio:
volumes:
- - "./care/media/minio:/data"
+ - "minio-data:/data"
- "./docker/minio/init-script.sh:/init-script.sh:ro"
- "./docker/minio/entrypoint.sh:/entrypoint.sh:ro"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - "./care/media/minio:/data" | |
| - "./docker/minio/init-script.sh:/init-script.sh:ro" # Mount the init script | |
| - "./docker/minio/entrypoint.sh:/entrypoint.sh:ro" # Mount the entrypoint script | |
| - "minio-data:/data" | |
| - "./docker/minio/init-script.sh:/init-script.sh:ro" # Mount the init script | |
| - "./docker/minio/entrypoint.sh:/entrypoint.sh:ro" # Mount the entrypoint script |
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
New Features
Bug Fixes
Documentation