-
Notifications
You must be signed in to change notification settings - Fork 845
Automatically start Docker service with --no-autostart disable option #518
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
b8ecd36 to
b99efe2
Compare
|
@thaJeztah Can you kindly take a look ? Do you think the feature would be useful? |
|
Oh! This one dropped off my list; I think we actually had a conversation internally to consider making this the default so that The current script tried to align with the general conventions / expectations of I had to dig back the discussion related to it, but I think it was when we made this change in the docs, which also was changed to automatically start the daemon; We could add an option to not start it if we decide to make that change ( @vvoland WDYT? |
ac8d41c to
7d92938
Compare
Thanks for adding more context. I don't have a strong opinion on whether this feature should be default/non-default. I would personally like a |
|
Yeah something like |
|
Heh, yeah, for context; the That said; it's it sure is REALLY convenient to use, so the script is unlikely to go away; we consider it an opinionated way to configure things, which may involve things like this (automatically start), and with the proper warnings in place ("know what you're opting into when using the script") both in the script, the "dry run" option to check what you're about to run, and warnings in our docs, I think that's perfectly fine to do. |
I have refactored it to make autostart the default behaviour with a |
vvoland
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.
Thanks, looks good to me. One last thing though - please change the new echos to output to stderr ❤️
Thank you for the review @vvoland. I have updated the suggested changes and pushed the commit. Please let me know if you have any questions. Cheers. |
|
Oh, could you please rebase the PR on top of the latest master (after #530 was merged)? The test in the CI pipeline wasn't being ran correctly before, so the script wasn't even ran for this PR 🙈 |
|
@vvoland rebased and pushed. Please let me know if you have any questions. Cheers. |
|
Looks like it fails in a container environment: https://github.com/docker/docker-install/actions/runs/18424536661/job/52594868483?pr=518 Also, can you please rebase so it doesn't actually include the merge commits? ❤️ |
22f4812 to
c71834f
Compare
@vvoland Did a clean rebase and fixed errors from CI earlier. Can you please approve the PR workflow runs so that I can check to make sure my release and fix went as expected? Thanks again for the feedback. Cheers. |
|
@vvoland Any update on this PR? |
|
Sorry, this one went off my radar! Approved the workflows |
c71834f to
2794aef
Compare
|
@vvoland sorry for the delayed reply. I have made some fixes and pushed a commit, Can you kindly approve the workflow so that I can test my changes. I did test them locally but would be good to test it on the Pipeline. Please let me know if you have any questions. Cheers. |
|
@vvoland Can you please approve the workflow ? I made fixes after the last failure. |
|
Sorry for missing! Approved |
472bcdb to
f76b0da
Compare
vvoland
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.
LGTM. @thaJeztah PTAL
|
Left some comments / thoughts; I gave it a quick go to look what it looked like with my suggestions, and opened a PR against your branch; if you merge that PR, the changes should show up in this PR, and we can review / discuss those changes; |
9aa53d9 to
10b5c89
Compare
This PR makes the Docker daemon start automatically after installation by default, with a --no-autostart opt-out flag. This addresses issue docker#124 and aligns with the "opinionated convenience" philosophy of get.docker.com. Changes: - Autostart enabled by default (AUTOSTART=1) - Added --no-autostart flag to opt-out - Smart systemd detection using /run/systemd/system directory check - Falls back to traditional service management (service/chkconfig/update-rc.d) - Uses sh_c pattern for proper dry-run support - All informational messages output to stderr The systemd detection checks for /run/systemd/system directory which only exists when systemd is actually running as PID 1. This prevents failures in container environments where systemctl is installed but systemd is not running. Tested across all supported distributions: - Ubuntu (20.04, 22.04, 24.04) - Debian (11, 12) - CentOS Stream (8, 9) - Fedora (40, 41) - RHEL/AlmaLinux (8, 9) Fixes docker#124 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Gajesh Bhat <gajeshbht@gmail.com>
10b5c89 to
7d4c5e2
Compare
Let's run the CI check and see if this works. Thanks for the detailed review. Cheers. |
thaJeztah
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.
LGTM, thank you!
but would like another review from @vvoland as I made some of the last changes 😅
|
Thank you both @vvoland @thaJeztah . It was fun collaborting on this PR. My understanding of docker has definitly improved after this.Looking forward to contributing more in the future. Cheers. |
Summary
This PR makes the Docker daemon start automatically after installation by default, with a
--no-autostartopt-out flag. This addresses issue #124 and aligns with the current design philosophy ofget.docker.com.Problem
Users need an option to automatically start the Docker daemon after installation, as service management differs between distributions. Currently, users must manually run
sudo systemctl start dockerandsudo systemctl enable dockerafter installation.Solution
The script now automatically starts and enables the Docker daemon by default:
--no-autostartflag to skip daemon startsystemctl start docker && systemctl enable dockerservice docker start && chkconfig docker on(orupdate-rc.d)Usage