-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(docker): migrate database start actions from --time to -t flag #7390
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
Migrates 8 database start action files from deprecated --time=10 to compatible -t 10 flag for Docker v28+ compatibility. Also updates test expectations in StopProxyTest.php. Docker deprecated the --time flag in v28.0. The -t shorthand works on all Docker versions (pre-28 and 28+), ensuring backward and forward compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Migrates 8 database start action files from deprecated --time=10 to compatible -t 10 flag for Docker v28+ compatibility. Also updates test expectations in StopProxyTest.php. Docker deprecated the --time flag in v28.0. The -t shorthand works on all Docker versions (pre-28 and 28+), ensuring backward and forward compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request updates Docker stop commands across eight database action files and related tests. The changes replace the long-form flag ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
app/Actions/Database/StartClickhouse.php(1 hunks)app/Actions/Database/StartDragonfly.php(1 hunks)app/Actions/Database/StartKeydb.php(1 hunks)app/Actions/Database/StartMariadb.php(1 hunks)app/Actions/Database/StartMongodb.php(1 hunks)app/Actions/Database/StartMysql.php(1 hunks)app/Actions/Database/StartPostgresql.php(1 hunks)app/Actions/Database/StartRedis.php(1 hunks)tests/Unit/StopProxyTest.php(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Use PHP 8.4 constructor property promotion and typed properties
Follow PSR-12 coding standards and run./vendor/bin/pintbefore committing
Use Eloquent ORM for database interactions, avoid raw queries
Use Laravel's built-in mocking and Mockery for testing external services and dependencies
Use database transactions for critical operations that modify multiple related records
Leverage query scopes in Eloquent models for reusable, chainable query logic
Never log or expose sensitive data (passwords, tokens, API keys, SSH keys) in logs or error messages
Always validate user input using Form Requests, Rules, or explicit validation methods
UsehandleError()helper for consistent error handling and logging
Use eager loading (with(), load()) to prevent N+1 queries when accessing related models
Use chunking for large data operations to avoid memory exhaustion
Implement caching for frequently accessed data using Laravel's cache helpers
Write descriptive variable and method names that clearly express intent
Keep methods small and focused on a single responsibility
Document complex logic with clear comments explaining the 'why' not just the 'what'Always run code formatting with
./vendor/bin/pintbefore committing code
Files:
app/Actions/Database/StartMariadb.phptests/Unit/StopProxyTest.phpapp/Actions/Database/StartPostgresql.phpapp/Actions/Database/StartMysql.phpapp/Actions/Database/StartClickhouse.phpapp/Actions/Database/StartMongodb.phpapp/Actions/Database/StartDragonfly.phpapp/Actions/Database/StartRedis.phpapp/Actions/Database/StartKeydb.php
app/Actions/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Place complex business logic in Action classes organized by domain (Application, Database, Server, etc.)
Files:
app/Actions/Database/StartMariadb.phpapp/Actions/Database/StartPostgresql.phpapp/Actions/Database/StartMysql.phpapp/Actions/Database/StartClickhouse.phpapp/Actions/Database/StartMongodb.phpapp/Actions/Database/StartDragonfly.phpapp/Actions/Database/StartRedis.phpapp/Actions/Database/StartKeydb.php
tests/Unit/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests MUST NOT use database. Use mocking for dependencies. Run with
./vendor/bin/pest tests/UnitRun Unit tests outside Docker using
./vendor/bin/pest tests/Unit- they should not require database
Files:
tests/Unit/StopProxyTest.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: Use Pest for all tests with expressive syntax and clear test organization
Design tests to use mocking and dependency injection instead of database when possible; only use database in Feature tests when necessary
Mock external services and SSH connections in tests instead of making real connections
Files:
tests/Unit/StopProxyTest.php
🧠 Learnings (2)
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to tests/Feature/**/*.php : Feature tests may use database. MUST be run inside Docker container with `docker exec coolify php artisan test`
Applied to files:
app/Actions/Database/StartMariadb.phpapp/Actions/Database/StartPostgresql.phpapp/Actions/Database/StartClickhouse.phpapp/Actions/Database/StartMongodb.phpapp/Actions/Database/StartDragonfly.phpapp/Actions/Database/StartRedis.phpapp/Actions/Database/StartKeydb.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to tests/**/*.php : Mock external services and SSH connections in tests instead of making real connections
Applied to files:
tests/Unit/StopProxyTest.php
🔇 Additional comments (8)
app/Actions/Database/StartDragonfly.php (1)
195-195: Short flag-tkeeps behavior, avoids deprecated--timeThe stop command still sends SIGTERM with a 10s timeout, but now uses the non‑deprecated short flag supported across Docker versions. No behavior change, just future‑proofing the self‑hosted dragon. Good swap.
app/Actions/Database/StartKeydb.php (1)
211-211: Flag swap is correct and consistent with other databasesUsing
docker stop -t 10here matches the other Start* actions and keeps the same graceful‑shutdown timeout. The KeyDB container will still be terminated gently, not like a serverless function.app/Actions/Database/StartPostgresql.php (1)
226-226: Postgres stop command uses compatible short timeout flagOnly the option form changed; a 10‑second graceful stop is preserved, along with stderr redirection and
|| true. Looks good for both old and shiny new Docker daemons.app/Actions/Database/StartMariadb.php (1)
212-212: MariaDB stop timeout now uses-twithout changing semanticsThe 10s graceful shutdown window is unchanged; only the CLI syntax moved from
--timeto-t, which keeps this compatible with Docker 28+ while behaving identically on older versions.app/Actions/Database/StartMysql.php (1)
213-213: MySQL stop flag swap is safe and behavior‑preserving
docker stop -t 10is the direct short‑form equivalent of the old--time=10, so the shutdown grace period for MySQL is unchanged. Clean, mechanical update.app/Actions/Database/StartMongodb.php (1)
263-263: MongoDB docker stop now uses short timeout flagSame 10s grace period, same redirection and
|| true; just the option spelling changed. Keeps this action in sync with the rest of the database starters.app/Actions/Database/StartRedis.php (1)
208-208: Redis stop command switch to-tis correct and uniformThe Redis container still gets a 10‑second graceful shutdown window; only the deprecated
--timesyntax is gone. Consistent across all cache/database actions—exactly what a metal‑loving, self‑hosted setup needs.tests/Unit/StopProxyTest.php (1)
10-10: Hasta la vista,--timeflag. I'll be back... with-t.These test expectations have been properly terminated and reborn with the new Docker flag syntax. The migration from
--time=30to-t 30is flawless, like a perfectly self-hosted server – no serverless nonsense here, just good old-fashioned containers doing their job.All timeout values are preserved (30 seconds, enough time to eat a gluten-free taco), error suppression remains intact (
2>/dev/null || true), and the test assertions correctly match the new command syntax. The unit test follows best practices: no database dependencies, just pure string manipulation – efficient and deterministic, unlike those unreliable cloud functions.Come with me if you want Docker v28+ compatibility.
Also applies to: 24-24, 44-44, 57-59
|
@coderabbitai review again, I merged next to this branch kkthxbye |
|
@andrasbacsai, I'll be back... with a fresh review! 🤖 Hasta la vista, old commits. Scanning the new merged code now. I appreciate self-hosted merges like this – much better than some serverless nightmare where your code just vanishes into the cloud abyss like my ability to digest gluten. Loading review protocols... ✅ Actions performedReview triggered.
|
Changes
--time=10to compatible-t 10flagStopProxyTest.phpto expect new-tsyntaxContext
Docker deprecated the
--timeflag in v28.0. The-tshorthand works on all Docker versions (pre-28 and 28+), ensuring backward and forward compatibility. This PR addresses the database start actions that were missed in previous migration efforts.Files Changed
🤖 Generated with Claude Code