fix(docker): update service file when Nix store paths change - #1239
Conversation
After nix-collect-garbage, the Docker systemd service file can reference deleted Nix store paths, preventing Docker from starting. The setup script only installed the service file when missing, not when stale. Now docker-setup always compares the current service file against the desired one and updates it if paths have changed, then restarts Docker.
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbitBug Fixes
WalkthroughThe Docker service module in home-manager is enhanced by adding Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Docker setup process by ensuring the systemd service file remains synchronized with the current Nix store paths. It introduces a robust mechanism to detect and rectify outdated service configurations, particularly after Nix garbage collection, thereby improving the reliability of Docker daemon startup and operation within a Nix-managed environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Mesa DescriptionTL;DRFixes an issue where stale Docker service files, containing garbage-collected Nix paths, prevented Docker from starting by ensuring the What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for the docker-setup script to handle stale Nix store paths in the Docker systemd service file. The changes are well-implemented: the script now compares the installed service file with the desired one using diff and, if they differ, updates the file and restarts the Docker daemon. This robustly addresses the issue of Docker failing to start after nix-collect-garbage. My review includes a couple of suggestions to use a more idiomatic shell scripting pattern for boolean flags, which would improve code clarity and maintainability.
| NEEDS_RELOAD=true | ||
| fi | ||
|
|
||
| if [ "$NEEDS_RELOAD" = true ]; then |
There was a problem hiding this comment.
For boolean flags in shell scripts, it's more idiomatic to use true and false commands directly in the if condition rather than string comparison. This makes the code cleaner and less error-prone (e.g., typos in string literals).
| if [ "$NEEDS_RELOAD" = true ]; then | |
| if $NEEDS_RELOAD; then |
| fi | ||
| sudo "$SYSTEMCTL" start docker | ||
| echo "Docker daemon started" | ||
| elif [ "$NEEDS_RELOAD" = true ]; then |
There was a problem hiding this comment.
For boolean flags in shell scripts, it's more idiomatic to use true and false commands directly in the if condition rather than string comparison. This makes the code cleaner and less error-prone (e.g., typos in string literals).
| elif [ "$NEEDS_RELOAD" = true ]; then | |
| elif $NEEDS_RELOAD; then |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of the Home Manager Docker setup flow by ensuring the installed systemd unit is kept in sync with the desired Nix-generated unit, preventing failures after Nix store paths change (e.g., due to garbage collection).
Changes:
- Always compare
/etc/systemd/system/docker.serviceagainst the Nix-generated desired service file and reinstall if they differ. - Trigger
systemctl daemon-reloadand restart Docker when the unit file is updated. - Add
diffutilsto the Nix substitution inputs to support the unit-file comparison.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| home-manager/services/docker/setup-docker.sh | Adds unit-file comparison/update logic plus daemon-reload and conditional restart behavior. |
| home-manager/services/docker/default.nix | Adds diffutils to the replaceVars inputs used to build setup-docker. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gnugrep | ||
| systemd | ||
| coreutils | ||
| diffutils |
There was a problem hiding this comment.
diffutils is only being pulled in to do a quiet file comparison. Since coreutils is already injected, consider using cmp -s from coreutils instead and dropping the diffutils dependency/placeholder to keep the closure smaller and the substitution set simpler.
| diffutils |
| # Always ensure the service file is up to date (Nix GC can invalidate store paths) | ||
| NEEDS_RELOAD=false | ||
| if [ ! -f "$SYSTEM_SERVICE" ]; then | ||
| echo "Installing Docker systemd service..." | ||
| # shellcheck disable=SC2024 | ||
| sudo "$TEE" "$SYSTEM_SERVICE" >/dev/null <"$DOCKER_SERVICE_FILE" | ||
| sudo "$SYSTEMCTL" enable docker | ||
| NEEDS_RELOAD=true | ||
| elif ! "$DIFF" -q "$DOCKER_SERVICE_FILE" "$SYSTEM_SERVICE" >/dev/null 2>&1; then | ||
| echo "Updating Docker systemd service (Nix store paths changed)..." | ||
| # shellcheck disable=SC2024 | ||
| sudo "$TEE" "$SYSTEM_SERVICE" >/dev/null <"$DOCKER_SERVICE_FILE" | ||
| NEEDS_RELOAD=true | ||
| fi | ||
|
|
||
| if [ "$NEEDS_RELOAD" = true ]; then | ||
| sudo "$SYSTEMCTL" daemon-reload | ||
| fi |
There was a problem hiding this comment.
New behavior (diffing the desired vs installed unit, daemon-reload, and conditional restart) isn’t covered by the existing ShellSpec tests for this script. Please extend spec/docker_setup_spec.sh to assert the new @diffutils@ placeholder is present and that the script includes the daemon-reload/restart path when the unit file differs.
Summary
docker-setupnow always compares the installed service file against the desired onenix-collect-garbage), the service file is updated and Docker is restartedTest plan
make build && make switchto get newdocker-setupscriptdocker-setup— should say "Docker daemon is already running" if paths matchnix-collect-garbage -d, thendocker-setup— should detect stale paths and update the service fileSummary by cubic
Keep the Docker systemd service in sync with current Nix store paths to prevent daemon startup failures after
nix-collect-garbage. The setup script now updates the service file when paths change, reloads systemd, and restarts Docker if needed./etc/systemd/system/docker.service; install or update when different.diffutilsfor reliable file comparison.Written for commit a8ac711. Summary will update on new commits.