Added support for different database provider #584
Conversation
|
@zpratikpathak is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces modifications to the development Dockerfile and the entrypoint script. The changes focus on improving the script's database provider detection mechanism by dynamically setting the provider based on the Changes
Sequence DiagramsequenceDiagram
participant Entrypoint as Entrypoint Script
participant Prisma as Prisma Schema
participant Database as Database Provider
Entrypoint->>Entrypoint: Parse DATABASE_URL
alt MySQL URL
Entrypoint->>Prisma: Set provider to "mysql"
else PostgreSQL URL
Entrypoint->>Prisma: Set provider to "postgresql"
else MariaDB URL
Entrypoint->>Prisma: Set provider to "mariadb"
else Invalid URL
Entrypoint->>Entrypoint: Print error and exit
end
Entrypoint->>Database: Prepare database connection
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: 2
🧹 Nitpick comments (1)
var/docker/entrypoint.sh (1)
Line range hint
29-61: Add error handling for critical operations.While the startup logic is correct, it would benefit from additional error handling for critical operations:
Apply this diff to improve error handling:
echo "Entrypoint: Running database migrations" -npm run prisma-db-push +if ! npm run prisma-db-push; then + echo "ERROR: Database migration failed" + exit 1 +fi -mkdir -p /etc/supervisor.d/ +if ! mkdir -p /etc/supervisor.d/; then + echo "ERROR: Failed to create supervisor directory" + exit 1 +fi if [[ "$INTERNAL_PROXY_ENABLED" != "false" ]]; then echo "Entrypoint: Starting internal proxy" - cp -vf /app/supervisord_available_configs/caddy.conf /etc/supervisor.d/ + if ! cp -vf /app/supervisord_available_configs/caddy.conf /etc/supervisor.d/; then + echo "ERROR: Failed to copy caddy configuration" + exit 1 + fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile.dev(3 hunks)var/docker/entrypoint.sh(2 hunks)
🔇 Additional comments (4)
var/docker/entrypoint.sh (1)
19-27: LGTM!The configuration handling logic is well-implemented with appropriate error handling and user feedback.
Dockerfile.dev (3)
27-27: LGTM!The entrypoint script setup follows best practices:
- Uses standard system binary location
- Sets proper permissions
- Uses absolute path in ENTRYPOINT
Also applies to: 38-42
Line range hint
48-82: LGTM!The production image setup follows Docker best practices with proper layering, volume management, and labeling.
17-19: Consider version handling strategy for Alpine packages.While pinning package versions is good practice, it could cause build failures if specific versions are removed from Alpine repositories. Consider:
- Using version ranges instead of exact versions
- Adding fallback versions
- Documenting version update process
Run this script to verify package availability:
| # Update the datasource provider in the schema.prisma file | ||
| sed -i "s/provider = \".*\"/provider = \"$provider\"/" ./libraries/nestjs-libraries/src/database/prisma/schema.prisma |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks for schema.prisma modification.
The current implementation lacks validation and error handling for the schema.prisma update:
Apply this diff to add necessary safety checks:
# Update the datasource provider in the schema.prisma file
-sed -i "s/provider = \".*\"/provider = \"$provider\"/" ./libraries/nestjs-libraries/src/database/prisma/schema.prisma
+SCHEMA_PATH="./libraries/nestjs-libraries/src/database/prisma/schema.prisma"
+
+if [ ! -f "$SCHEMA_PATH" ]; then
+ echo "ERROR: schema.prisma not found at $SCHEMA_PATH"
+ exit 1
+fi
+
+# Create backup
+cp "$SCHEMA_PATH" "${SCHEMA_PATH}.bak"
+
+# Update provider
+if ! sed -i "s/provider = \".*\"/provider = \"$provider\"/" "$SCHEMA_PATH"; then
+ echo "ERROR: Failed to update provider in schema.prisma"
+ mv "${SCHEMA_PATH}.bak" "$SCHEMA_PATH"
+ exit 1
+fi
+
+# Clean up backup on success
+rm "${SCHEMA_PATH}.bak"📝 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.
| # Update the datasource provider in the schema.prisma file | |
| sed -i "s/provider = \".*\"/provider = \"$provider\"/" ./libraries/nestjs-libraries/src/database/prisma/schema.prisma | |
| # Update the datasource provider in the schema.prisma file | |
| SCHEMA_PATH="./libraries/nestjs-libraries/src/database/prisma/schema.prisma" | |
| if [ ! -f "$SCHEMA_PATH" ]; then | |
| echo "ERROR: schema.prisma not found at $SCHEMA_PATH" | |
| exit 1 | |
| fi | |
| # Create backup | |
| cp "$SCHEMA_PATH" "${SCHEMA_PATH}.bak" | |
| # Update provider | |
| if ! sed -i "s/provider = \".*\"/provider = \"$provider\"/" "$SCHEMA_PATH"; then | |
| echo "ERROR: Failed to update provider in schema.prisma" | |
| mv "${SCHEMA_PATH}.bak" "$SCHEMA_PATH" | |
| exit 1 | |
| fi | |
| # Clean up backup on success | |
| rm "${SCHEMA_PATH}.bak" |
| # Determine the provider based on the DATABASE_URL | ||
| if [[ "$DATABASE_URL" == mysql://* ]]; then | ||
| provider="mysql" | ||
| elif [[ "$DATABASE_URL" == postgresql://* ]]; then | ||
| provider="postgresql" | ||
| elif [[ "$DATABASE_URL" == mariadb://* ]]; then | ||
| provider="mariadb" | ||
| else | ||
| echo "Unsupported database provider in DATABASE_URL" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance database provider detection robustness.
While the provider detection logic works, it could be more robust:
- Add validation for empty/unset DATABASE_URL
- Support case-insensitive matching
- Provide more informative error message listing supported providers
Apply this diff to improve the implementation:
# Determine the provider based on the DATABASE_URL
+if [ -z "$DATABASE_URL" ]; then
+ echo "ERROR: DATABASE_URL environment variable is not set"
+ exit 1
+fi
+
+database_url_lower=$(echo "$DATABASE_URL" | tr '[:upper:]' '[:lower:]')
-if [[ "$DATABASE_URL" == mysql://* ]]; then
+if [[ "$database_url_lower" == mysql://* ]]; then
provider="mysql"
-elif [[ "$DATABASE_URL" == postgresql://* ]]; then
+elif [[ "$database_url_lower" == postgresql://* ]]; then
provider="postgresql"
-elif [[ "$DATABASE_URL" == mariadb://* ]]; then
+elif [[ "$database_url_lower" == mariadb://* ]]; then
provider="mariadb"
else
- echo "Unsupported database provider in DATABASE_URL"
+ echo "ERROR: Unsupported database provider in DATABASE_URL"
+ echo "Supported providers: mysql://, postgresql://, mariadb://"
exit 1
fi📝 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.
| # Determine the provider based on the DATABASE_URL | |
| if [[ "$DATABASE_URL" == mysql://* ]]; then | |
| provider="mysql" | |
| elif [[ "$DATABASE_URL" == postgresql://* ]]; then | |
| provider="postgresql" | |
| elif [[ "$DATABASE_URL" == mariadb://* ]]; then | |
| provider="mariadb" | |
| else | |
| echo "Unsupported database provider in DATABASE_URL" | |
| exit 1 | |
| fi | |
| # Determine the provider based on the DATABASE_URL | |
| if [ -z "$DATABASE_URL" ]; then | |
| echo "ERROR: DATABASE_URL environment variable is not set" | |
| exit 1 | |
| fi | |
| database_url_lower=$(echo "$DATABASE_URL" | tr '[:upper:]' '[:lower:]') | |
| if [[ "$database_url_lower" == mysql://* ]]; then | |
| provider="mysql" | |
| elif [[ "$database_url_lower" == postgresql://* ]]; then | |
| provider="postgresql" | |
| elif [[ "$database_url_lower" == mariadb://* ]]; then | |
| provider="mariadb" | |
| else | |
| echo "ERROR: Unsupported database provider in DATABASE_URL" | |
| echo "Supported providers: mysql://, postgresql://, mariadb://" | |
| exit 1 | |
| fi |
|
it's a big change to check, let see if this pull requests get more upvotes! |
What kind of change does this PR introduce?
Database support for different providers
eg: Bug fix, feature, docs update, ...
Why was this change needed?
Application was not able to automatically adjust itself to different provider based on the "DATABASE" string, it was hardcoded, if someone wants to use mariadb they may need to pull the image, get inside the image and change the provider. It would have been a lot better if it can automatically set the provider in prisma
Please link to related issues when possible, and explain WHY you changed things, not WHAT you changed.
Other information:
eg: Did you discuss this change with anybody before working on it (not required, but can be a good idea for bigger changes).
I discussed with the fellow Develop relations, since they already had mysql/mariadb and they didn't want to spin up a new PostgreSQL they thought it was a much better idea, they were already doing image modification in order to support mariadb as provider
Any plans for the future, etc?
I also want to add the guideline in docs how to choose different database and add some sample docker-compose file with different database
Checklist:
Put a "X" in the boxes below to indicate you have followed the checklist;
Summary by CodeRabbit