-
Notifications
You must be signed in to change notification settings - Fork 2
Add MySQL Docker Compose profile with isolated services #253
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
Conversation
- Add MySQL 8.0 database service with health checks and profile - Add mysql-test service for isolated testing on port 33306 - Add llm-proxy-mysql service variant with MySQL configuration - Update Dockerfile to support MYSQL_SUPPORT build argument - Add MySQL volumes (mysql_data and mysql_test_data) - Update .env.example with MySQL connection documentation and credentials Co-authored-by: mfittko <326798+mfittko@users.noreply.github.com>
- Create comprehensive MySQL Docker setup guide - Document build tags, configuration, and usage - Add troubleshooting and security considerations - Update database documentation index with MySQL reference Co-authored-by: mfittko <326798+mfittko@users.noreply.github.com>
| | Use Case | Recommended Database | | ||
| |----------|---------------------| | ||
| | Development/Testing | SQLite (default) | | ||
| | Single instance production | SQLite or PostgreSQL | |
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.
this should also be including MySQL
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.
Fixed in 42d83f7. Updated the AWS ECS deployment line to include "Aurora PostgreSQL Serverless v2 or Aurora MySQL".
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.
Pull request overview
This PR adds MySQL 8.0 as an optional database backend for llm-proxy, following the existing PostgreSQL pattern. The implementation uses Docker Compose profiles for opt-in activation, ensuring no conflicts with existing SQLite/PostgreSQL deployments.
Key Changes:
- MySQL Docker services with health checks and isolated test instances
- Conditional MySQL driver compilation via Dockerfile build arguments
- Comprehensive setup and troubleshooting documentation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yml | Adds mysql and mysql-test services with profiles, plus llm-proxy-mysql variant on port 8082; includes persistent volumes for both dev and test instances |
| Dockerfile | Implements MYSQL_SUPPORT build argument with shell logic to combine postgres and mysql build tags conditionally |
| .env.example | Documents MySQL connection string format and adds MYSQL_ROOT_PASSWORD, MYSQL_PASSWORD, MYSQL_DATABASE variables |
| docs/database/index.md | Updates database selection guide to include MySQL as an option for multi-instance production |
| docs/database/docker-compose-mysql.md | New comprehensive guide covering build tags, configuration, integration tests, troubleshooting, and MySQL-specific setup |
.env.example
Outdated
| # | ||
| # MySQL Format: user:password@tcp(host:port)/database?parseTime=true | ||
| # MySQL Example: llmproxy:secret@tcp(localhost:3306)/llmproxy?parseTime=true | ||
| # SECURITY: For production, use TLS connection: ?tls=true or ?tls=skip-verify (for self-signed) |
Copilot
AI
Jan 1, 2026
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.
This comment recommends using the MySQL DSN parameter tls=skip-verify for production, which disables certificate validation and allows a man-in-the-middle on the network path to impersonate the database server and read or tamper with data. For production, tls=skip-verify should be discouraged and documentation should direct users to use tls=true or a properly configured custom CA (tls=custom) so the client verifies the server certificate.
| # SECURITY: For production, use TLS connection: ?tls=true or ?tls=skip-verify (for self-signed) | |
| # SECURITY: For production, use TLS with certificate verification: ?tls=true (or a properly configured custom TLS/CA). ?tls=skip-verify disables verification and must not be used in production. |
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.
Fixed in 42d83f7. Updated the security comment to explicitly discourage tls=skip-verify in production and recommend tls=true or properly configured custom TLS/CA instead.
mfittko
left a comment
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.
PR Review: MySQL Docker Compose Profile Implementation
Summary
This PR successfully implements the core MySQL Docker Compose infrastructure as specified in issue #243. The implementation follows the existing PostgreSQL pattern well and includes comprehensive documentation. However, the PR description contains inaccuracies that need addressing, and there are unresolved review comments.
✅ What's Done Well
Docker Compose Services (All Tasks Complete)
| Requirement | Status | Notes |
|---|---|---|
| MySQL service with mysql:8.0 | ✅ | Correctly configured on port 3306 |
| mysql-test service | ✅ | Isolated on port 33306 |
| Health checks | ✅ | mysqladmin ping with proper timings |
| Named volumes | ✅ | mysql_data and mysql_test_data |
| Profile isolation | ✅ | mysql and mysql-test profiles |
| llm-proxy-mysql service | ✅ | Port 8082, depends on mysql healthy |
Dockerfile Enhancement
- Build tag logic properly combines
postgresand/ormysqltags MYSQL_SUPPORTbuild argument follows existing pattern- Shell script logic handles both/either/neither cases correctly
Environment Configuration
- MySQL credentials added to
.env.example - Connection string format documented with DSN examples
- Docker Compose variables added with defaults
Documentation
- Comprehensive 380-line guide covering build tags, setup, testing, troubleshooting
- Database index updated to include MySQL as an option
- Security considerations and performance tuning documented
❌ Issues Requiring Attention
1. PR Description Inaccuracies (Critical)
The PR description claims changes that do not exist in the diff:
| Claimed Change | Actual Status |
|---|---|
admin-mysql service on port 8083 |
❌ Not in code |
cmd/proxy/server.go DB driver switch |
❌ Not in diff |
.github/workflows/docker.yml CI update |
❌ Not in diff |
| Bug fix for Admin UI tokens list | ❌ Not in diff |
Action Required: Update PR description to accurately reflect the actual changes, or add the missing changes.
2. Unresolved Review Comments
| Comment | Location | Status |
|---|---|---|
Security concern: tls=skip-verify documentation |
.env.example:30 |
|
| MySQL should be included for AWS ECS | docs/database/index.md:25 |
The security comment about tls=skip-verify being recommended for production is valid - this should be reworded to discourage skip-verify in production.
3. Missing Trailing Newline (Minor)
docker-compose.yml is missing a trailing newline at EOF.
Acceptance Criteria Verification
Per issue #243:
| Criterion | Status |
|---|---|
MySQL service starts with docker compose --profile mysql up |
✅ |
| Health check passes within start period | ✅ |
| Data persists across container restarts | ✅ |
| LLM proxy connects to MySQL when configured | ✅ |
| mysql-test works independently on port 33306 | ✅ |
| Port mapping allows local MySQL client connections | ✅ |
| No conflicts with existing PostgreSQL/SQLite setups | ✅ |
| Named volumes properly declared | ✅ |
Environment variables documented in .env.example |
✅ |
CI Status
Status checks are pending - should verify tests pass before merge.
Verdict: Request Changes
The core Docker Compose implementation is solid and follows best practices, but:
- The PR description is misleading - it claims several changes that aren't in the diff
- Unresolved review comments need to be addressed before merge
- CI checks should complete successfully
Once the PR description is corrected and review comments are addressed, this is ready to merge.
Recommendations for Author
- Update PR description to match actual changes (remove references to non-existent
admin-mysql,server.go, and workflow changes) - Address the
tls=skip-verifysecurity documentation concern - Consider adding Aurora MySQL to the AWS ECS line or respond to the review comment
- Add trailing newline to
docker-compose.yml - Verify CI passes
…mmendations - Fix .env.example to discourage tls=skip-verify in production (addresses security concern) - Add Aurora MySQL to AWS ECS deployment recommendations - Add trailing newline to docker-compose.yml Co-authored-by: mfittko <326798+mfittko@users.noreply.github.com>
mfittko
left a comment
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.
✅ Approved - MySQL Docker Compose Profile Implementation
All issues from the previous review have been addressed. The implementation is solid and ready to merge.
Changes Since Last Review
| Issue | Resolution |
|---|---|
| PR description inaccuracies | ✅ Fixed - Now accurately reflects actual changes |
Security: tls=skip-verify documentation |
✅ Fixed in .env.example - Now explicitly discourages skip-verify in production |
| Aurora MySQL for AWS ECS | ✅ Fixed in docs/database/index.md - Added "Aurora MySQL" option |
| Missing trailing newline | ✅ Fixed in docker-compose.yml |
Implementation Quality
Docker Compose Services ✅
mysql- Development database with health checks on port 3306mysql-test- Isolated test instance on port 33306llm-proxy-mysql- Proxy variant on port 8082- Proper profile isolation (
mysql,mysql-test) - Named volumes for data persistence
Dockerfile ✅
MYSQL_SUPPORTbuild argument properly implemented- Build tag logic correctly combines postgres and/or mysql tags
Documentation ✅
- Comprehensive 380-line guide covering all aspects
- Security considerations properly documented
- TLS guidance correct
Environment Configuration ✅
- MySQL credentials and connection string documented
- Security warnings about TLS are appropriate
Acceptance Criteria (Issue #243)
| Criterion | Status |
|---|---|
MySQL service starts with docker compose --profile mysql up |
✅ |
| Health check passes within start period | ✅ |
| Data persists across container restarts | ✅ |
| LLM proxy connects to MySQL when configured | ✅ |
| mysql-test works independently on port 33306 | ✅ |
| Port mapping allows local MySQL client connections | ✅ |
| No conflicts with existing PostgreSQL/SQLite setups | ✅ |
| Named volumes properly declared | ✅ |
Environment variables documented in .env.example |
✅ |
Note
CI status is pending - please ensure tests pass before merging. Integration tests for MySQL are scheduled separately in #245.
Good work addressing the review feedback! 🎉
Adds MySQL 8.0 as an optional database backend alongside PostgreSQL and SQLite, with dedicated Docker Compose services for development and testing.
Changes
Docker Compose Services
mysql(profile:mysql) - Development database on port 3306 with health checks and persistent volumemysql-test(profile:mysql-test) - Isolated test instance on port 33306 with separate volumellm-proxy-mysql(profile:mysql) - Proxy variant on port 8082 using MySQL backend, depends on mysql healthDockerfile
MYSQL_SUPPORTbuild argument for conditional MySQL driver compilationpostgres,mysql, or both)Environment Configuration
MYSQL_ROOT_PASSWORD,MYSQL_PASSWORD,MYSQL_DATABASE) to.env.exampleuser:password@tcp(host:port)/database?parseTime=truetls=skip-verifyin production and recommendtls=trueor properly configured custom TLS/CADocumentation
docs/database/docker-compose-mysql.mdwith setup, configuration, and troubleshooting guidesUsage
Port Allocation
Follows existing PostgreSQL pattern for consistency. No conflicts with default services (SQLite/PostgreSQL on ports 5432, 8080, 8081).
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.