-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat: NixOS support #7170
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
base: next
Are you sure you want to change the base?
feat: NixOS support #7170
Conversation
f96648f to
b5cab2e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes add comprehensive NixOS support to the application, including OS detection and package manager mapping, update checking with NixOS-specific output parsing, Docker installation guidance, package update handling, validation messaging, and UI components for displaying system-wide updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant CheckUpdates
participant parseNixosOutput
Server->>CheckUpdates: checkUpdates()
activate CheckUpdates
CheckUpdates->>CheckUpdates: Detect OS: NixOS
CheckUpdates->>CheckUpdates: Map to package_manager: nixos
CheckUpdates->>CheckUpdates: Execute nix-channel update<br/>nixos-rebuild switch
note over CheckUpdates: Capture command output
CheckUpdates->>parseNixosOutput: parseNixosOutput(output)
activate parseNixosOutput
alt "these N paths will be fetched" found
parseNixosOutput->>parseNixosOutput: Extract package_count = N
parseNixosOutput-->>CheckUpdates: updates[] with nixos-system,<br/>is_system_update: true,<br/>package_count: N
else Changes detected (building/fetching)
parseNixosOutput-->>CheckUpdates: updates[] with generic entry,<br/>package_count: "unknown"
else No changes detected
parseNixosOutput-->>CheckUpdates: empty updates[]
end
deactivate parseNixosOutput
CheckUpdates-->>Server: Return {<br/> total_updates,<br/> updates,<br/> is_nixos: true<br/>}
deactivate CheckUpdates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 2
🧹 Nitpick comments (7)
templates/service-templates-latest.json (1)
3680-3680: Base64-encoded compose field is difficult to review.The
composefield contains a large base64-encoded Docker Compose configuration that cannot be inspected directly. If this change includes NixOS-specific Docker setup instructions, consider adding a decoded version as a comment or in supplementary documentation for reviewability.tests/Unit/NixosPatchCheckTest.php (1)
20-66: parseNixosOutput tests cover key scenarios wellThe remaining tests exercise the NixOS parser for:
- Explicit “these N paths will be fetched” output
- Generic “changes present but no count” output
- “System is up to date” output
Expectations on
total_updates,package,package_count,description, andis_nixosmatch the parser’s contract. This gives good confidence in the NixOS path.You could add
is_nixosassertions to the latter two tests as well for extra safety.app/Livewire/Server/ValidateAndInstall.php (1)
107-112: NixOS‑specific validation messages are consistent and non‑blockingThe new NixOS branches in
validateOSandvalidateDockerEngine:
- Surface clear guidance that Docker must be configured via NixOS configuration.
- Persist those messages to
validation_logs.- Keep the validation flow running so the Docker install step can emit its own NixOS instructions and logs.
The behavior is coherent and doesn’t regress other OS types.
If you want to reduce confusion, you could slightly deduplicate/align the NixOS error texts between
validateOSandvalidateDockerEngineso users see exactly the same wording across steps.Also applies to: 124-128, 144-148
resources/views/livewire/server/security/patches.blade.php (1)
21-22: NixOS patch UI clearly communicates system‑wide update impactThe Blade changes:
- Correctly list
nixosas a supported package manager in the helper.- Add a dedicated NixOS notice explaining atomic system‑wide updates and potential service impact.
- Use
is_system_update,description, andpackage_countto distinguish system rebuild entries and show extra context.- Swap per‑row “Update” for a global “Update System” action when
$packageManager === 'nixos', matching the backend’s system‑wide update behavior.The logic and conditionals look sound and don’t affect non‑NixOS flows.
Longer term, you might consider hiding the per‑row action column entirely for NixOS (since all rows map to the same system update) to reduce visual duplication.
Also applies to: 46-65, 73-73, 93-95, 107-111, 120-122, 126-132
app/Actions/Server/CheckUpdates.php (1)
59-61: NixOS update detection and parsing are reasonable and aligned with the UI
- OS normalization and package manager mapping for
nixosare straightforward and don’t affect other OS types.- The NixOS branch runs
nix-channel --update nixosfollowed bynixos-rebuild dry-build 2>&1, then parses the combined output, returningosIdandpackage_managerlike the other branches.parseNixosOutput():
- Detects
these N paths will be fetchedand emits a singlenixos-systemupdate withis_system_update = trueand a numericpackage_count.- Falls back to a generic
nixos-systementry when it sees build/fetch/unpack activity but no explicit count, markingpackage_countas'unknown'.- Returns no updates when there are no such indicators.
- Always marks
is_nixos = true, which matches how the consumer and tests use it.Overall the logic is correct and matches the new tests and UI behavior.
You might consider making the string checks (
'paths will be fetched','building','fetching','unpacking') case‑insensitive or slightly more structured if you notice NixOS output variations in the wild, but the current implementation is fine as a first iteration.Also applies to: 73-75, 100-109, 236-289
tests/Unit/NixosServerSetupTest.php (1)
19-33: getNixosDockerInstallCommand test validates key messagingThe reflection-based test for
getNixosDockerInstallCommand()correctly checks that the generated command includes:
- A NixOS Docker configuration guide label,
virtualisation.dockersettings withenable = true,- A
nixos-rebuild switchinstruction.That anchors the method’s output contract without coupling to the full exact string.
In the future, you might factor the guide text into a separate formatter so it can be tested without reflection, but this is acceptable for now.
app/Actions/Server/InstallDocker.php (1)
99-105: NixOS Docker install path is guidance‑only and meshes with validation flow
- The new NixOS branch in the prerequisites section checks for
docker,git, andjqand prints clear messages instead of attempting to install them, which is appropriate for NixOS’s declarative model.- When NixOS is detected later, the installer uses
getNixosDockerInstallCommand(), which prints a concise configuration.nix snippet (virtualisation.docker = { enable = true; ... }, packages list, and anixos-rebuild switchhint) and instructs the user to click “Retry” afterwards.- This lines up with the NixOS‑specific messages in
ValidateAndInstalland avoids trying to imperatively install Docker on NixOS.Based on learnings: if you observe failures on NixOS due to the shared
jq//etc/docker/daemon.jsonconfiguration block later in this method, consider gating that block for non‑NixOS systems or wrapping it with an explicitcommand -v jqguard so the NixOS path degrades more gracefully without requiringjqto be preinstalled.Also applies to: 119-121, 194-218
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
app/Actions/Server/CheckUpdates.php(4 hunks)app/Actions/Server/InstallDocker.php(3 hunks)app/Actions/Server/UpdatePackage.php(1 hunks)app/Livewire/Server/ValidateAndInstall.php(3 hunks)bootstrap/helpers/constants.php(1 hunks)resources/views/livewire/server/security/patches.blade.php(5 hunks)templates/service-templates-latest.json(3 hunks)templates/service-templates.json(3 hunks)tests/Unit/NixosPatchCheckTest.php(1 hunks)tests/Unit/NixosServerSetupTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-11T15:54:46.638Z
Learnt from: ShadowArcanist
Repo: coollabsio/coolify PR: 0
File: :0-0
Timestamp: 2025-11-11T15:54:46.638Z
Learning: For Coolify installer scripts: Avoid adding dependencies on commands that aren't universally available across all supported distributions (Ubuntu, Debian, Arch, Alpine, CentOS, Fedora, RHEL, etc.). Prefer built-in commands or install required tools within the script.
Applied to files:
app/Actions/Server/InstallDocker.php
🔇 Additional comments (5)
templates/service-templates.json (1)
985-985: Clarify the relationship between category field updates and NixOS support.The changes add
"category": "analytics"to service template entries (embyStat at line 985, rybbit at line 3689), but the PR objectives focus on NixOS host support, Docker installation guidance, and patch checking. These JSON updates appear orthogonal to the stated PR goals.Verify that these category field additions are intentional and necessary as part of this PR, or consider splitting them into a separate, dedicated PR for better change isolation and review clarity.
Also applies to: 3689-3689
templates/service-templates-latest.json (2)
976-988: Unclear relationship between changes and NixOS support objectives.The provided hunks show generic service template metadata updates (adding
categoryfields and updating tags for EmbyStat and Rybbit services), but no code related to NixOS support is visible. Per the PR objectives, this PR should enable NixOS host configuration, Docker installation guidance, and patch checking—none of which appear to be addressed in these template changes.Could you clarify:
- Are there additional file changes (e.g., the
detected_manifestssection mentioned in the AI summary) that are not shown in the provided hunks?- How do these service template metadata changes relate to the NixOS support feature described in the PR objectives?
Also applies to: 3680-3692
976-988: Summary inconsistency: mention ofdetected_manifestschanges.The AI-generated summary references changes to a
detected_manifestsblock and manifest pattern detection, but no such changes appear in the provided hunks. Please verify whether the full scope of file changes has been captured.Also applies to: 3680-3692
app/Actions/Server/UpdatePackage.php (1)
36-39: NixOS update path matches system‑wide rebuild modelUsing
nix-channel --update nixos && nixos-rebuild switchfor both “all” and “single” updates is consistent with NixOS’s atomic, system‑wide model and aligns with the NixOS‑specific UI that only offers system updates. No issues here.bootstrap/helpers/constants.php (1)
60-67: Includingnixosin SUPPORTED_OS is consistent with new handlingAdding
nixoshere keeps OS detection aligned with the new NixOS branches in validation, updates, and Docker install. Looks good.
Resolved conflicts in: - app/Actions/Server/InstallDocker.php: Integrated NixOS support with refactored prerequisite installation - app/Livewire/Server/ValidateAndInstall.php: Added NixOS detection and error messages to new prerequisite validation flow
Changes
Summary by CodeRabbit
New Features
Refactor
Tests