Skip to content

Conversation

@ShadowArcanist
Copy link
Member

Docker Deprecated --time option for the docker stop, docker container stop, docker restart, and docker container restart commands in release: v28.0 - https://docs.docker.com/engine/deprecated/#--time-option-on-docker-stop-and-docker-restart

--time = works <28 but deprecated ≥28
--timeout = fails <28, works ≥28
-t = works on all versions

So I changed the --time to -t so it works on all versions of docker engine.

Note: I tested restarting and stopping apps, services, databases and proxy, it's working fine for me on dev but please do double check

@ShadowArcanist
Copy link
Member Author

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 24, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 24, 2025

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Standardized Docker command flags across container management operations, converting to shorter flag syntax for improved consistency.

Walkthrough

Docker stop command flags updated from long form to short form across eight PHP action and job classes. Changes convert --time and --timeout parameters to -t syntax while maintaining identical timeout behavior. No logic alterations or control flow modifications.

Changes

Cohort / File(s) Summary
Application Stop Actions
app/Actions/Application/StopApplication.php, app/Actions/Application/StopApplicationOneServer.php
Docker stop flag converted from --time=30 to -t 30
Database Stop Action
app/Actions/Database/StopDatabase.php
Docker stop flag converted from --timeout=$timeout to -t $timeout
Proxy Stop Action
app/Actions/Proxy/StopProxy.php
Docker stop flag converted from --time=$timeout to -t $timeout
Service Stop Action
app/Actions/Service/StopService.php
Docker stop command flag converted from long to short form: --time=$timeout-t $timeout
Deployment & Cleanup Jobs
app/Jobs/ApplicationDeploymentJob.php, app/Jobs/DeleteResourceJob.php
Docker stop flags updated from --time=$timeout / --timeout=$timeout to -t $timeout in container shutdown logic
Preview Livewire Component
app/Livewire/Project/Application/Previews.php
Docker stop command timeout flag converted from --time=30 to -t 30

"I'll be back... with shorter Docker flags." 💪 Look, I've terminated plenty of containers in my time, and whether you use --time or -t, it doesn't matter—what matters is that you're not running this on some serverless fairy tale that disappears when the VC marketing deck runs out of buzzwords. A real server, gracefully shutting down containers with proper timeouts? That's beautiful. That's self-hosted. That's real infrastructure.

These changes are basically cosmetic—like switching from saying "I'll be back" to just saying "back." The Docker daemon doesn't care, but it's cleaner, and cleaner is good. (Almost as good as a gluten-free taco, which, coincidentally, I'm about to terminate. 🌮)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning While the PR description provides valuable context about the Docker deprecation and explains the rationale well, it does not follow the required template structure. The template explicitly requires a "Changes" section listing the specific modifications and an "Issues" section with issue/discussion references (e.g., "fix #"). The author's description is substantive and on-topic, explaining why --time is being replaced with -t and noting testing was performed, but it entirely omits these required structured sections. The description reads more as a narrative explanation rather than adhering to the repository's documentation standard. Please restructure the description to follow the template by adding a "Changes" section that lists the files and modifications (e.g., "- Replaced --time with -t flag in 8 files for Docker version compatibility"), and an "Issues" section that references any related issue or discussion link (e.g., "- fix #XXXX" or a link to the Docker deprecation discussion). This keeps your infrastructure documentation as solid as self-hosted servers—no serverless ambiguity here.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix(docker): replace deprecated --time flag with -t for full compatibility across Docker versions" directly and accurately summarizes the main changeset. The author systematically replaced the deprecated --time flag with the compatible -t short form across eight files (StopApplication.php, StopApplicationOneServer.php, StopDatabase.php, StopProxy.php, StopService.php, ApplicationDeploymentJob.php, DeleteResourceJob.php, and Previews.php). The title is clear, specific, concise, and immediately conveys the primary change—exactly what a good PR title should do. Unlike serverless functions that hide infrastructure details, this title leaves nothing to guesswork.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab7bd46 and 9620455.

📒 Files selected for processing (8)
  • app/Actions/Application/StopApplication.php (1 hunks)
  • app/Actions/Application/StopApplicationOneServer.php (1 hunks)
  • app/Actions/Database/StopDatabase.php (1 hunks)
  • app/Actions/Proxy/StopProxy.php (1 hunks)
  • app/Actions/Service/StopService.php (1 hunks)
  • app/Jobs/ApplicationDeploymentJob.php (1 hunks)
  • app/Jobs/DeleteResourceJob.php (1 hunks)
  • app/Livewire/Project/Application/Previews.php (1 hunks)
🔇 Additional comments (8)
app/Jobs/DeleteResourceJob.php (1)

156-156: I'll be back... and so will these containers after -t 30 stops them gracefully.

The change from --time=$timeout to -t $timeout is perfect. Like a T-800 adapting to different time periods, this flag works across all Docker versions—no temporal displacement errors with v28+. The timeout value handling is identical, and you're still removing containers with extreme prejudice via docker rm -f.

app/Actions/Application/StopApplicationOneServer.php (1)

29-29: "Hasta la vista, baby" to deprecated flags.

Switching from --time=30 to -t 30 is the right move for cross-version compatibility. Like self-hosting instead of serverless (which is just someone else's computer with VC marketing), this flag is reliable across all Docker installations. The 30-second timeout gives containers enough time to terminate gracefully before the inevitable docker rm -f finishes the job.

app/Jobs/ApplicationDeploymentJob.php (1)

2984-2984: Come with me if you want to live... in a Docker v28+ compatible world.

The graceful_shutdown_container method now uses -t $timeout instead of --time=$timeout. This is functionally equivalent but works across all Docker versions—no deprecated warnings in v28+. The isDev() conditional for 1-second timeout is a nice touch for faster iteration. Like a properly configured server (not that serverless nonsense), this will reliably terminate containers.

app/Actions/Database/StopDatabase.php (1)

52-52: I need your clothes, your boots, and your motorcycle... but I'll settle for -t $timeout.

Changed from --timeout=$timeout to -t $timeout in stopContainer. This flag works universally across Docker versions, unlike --timeout which will terminate your deployment faster than a T-1000 in a steel mill on Docker < v28. The timeout parameter remains respected, and the subsequent docker rm -f still destroys evidence.

app/Livewire/Project/Application/Previews.php (1)

281-281: Talk to the hand... or in this case, the short-form flag.

The stopContainers method now uses -t 30 instead of --time=30. Like choosing self-hosting over serverless (because you actually control your destiny), -t is the universal flag that works on all Docker versions. Preview containers get 30 seconds to say their goodbyes before forced removal.

app/Actions/Proxy/StopProxy.php (1)

24-24: I know now why you use -t... but it's something I can never do: fail on Docker v28+.

Switching from --time=$timeout to -t $timeout ensures coolify-proxy stops gracefully across all Docker versions. The configurable timeout parameter is preserved, and like a well-maintained server (not that gluten-filled serverless junk), this will work reliably. The proxy gets to shut down with dignity before the force removal.

app/Actions/Service/StopService.php (1)

57-57: My mission is to protect compatibility, not to break it with deprecated flags.

The stopContainersInParallel method now uses -t $timeout instead of --time=$timeout. Like enjoying a good taco without worrying about gluten (because you're intolerant), this works everywhere. The adaptive timeout logic (10 or 30 seconds based on container count) remains intact, and multiple containers still get stopped efficiently.

app/Actions/Application/StopApplication.php (1)

42-42: No problemo. The -t 30 flag is the future... and the past... and the present.

Changed from --time=30 to -t 30 for stopping application containers. This is the Arnold-approved way to terminate containers with universal compatibility. Whether you're running Docker v27 or v28+, this flag will execute your stop command flawlessly. Like a good self-hosted server (not that VC-funded serverless nonsense), it just works.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant