feat: ini iso - #1471
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds new NixOS host configurations for VM and ISO image builds with shared module extraction, introduces Make targets for building and running VMs and ISOs, refactors the existing "matic" host configuration into shared modules, and adds minor shell script logic updates for credential handling and backend selection ordering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Mesa DescriptionTL;DRAdded initial setup for ISO. What changed?File-level changes are not available. 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 new NixOS configurations for the 'matic' and 'viper' hosts, including support for building live ISOs and a shared 'linux-base' module to streamline configuration. It also adds Makefile targets for building and running VMs and ISOs. Key feedback includes ensuring Makefile recipes use '&&' to prevent execution on build failure and resolving a username inconsistency in the evaluation tests for the 'matic' ISO.
| $(MAKE) build-vm HOST="$$host"; \ | ||
| script=$$(find ./result/bin -maxdepth 1 -type f \( -name 'run-*-vm' -o -name 'run-nixos-vm' \) | head -n 1); \ |
There was a problem hiding this comment.
The use of ; between the build command and the script lookup allows the recipe to continue even if the build fails. This can lead to executing a stale VM launcher from a previous build or a different host if ./result still exists. Use && to ensure the build succeeded before proceeding.
$(MAKE) build-vm HOST="$$host" && \
script=$$(find ./result/bin -maxdepth 1 -type f \( -name 'run-*-vm' -o -name 'run-nixos-vm' \) | head -n 1); \
| $(NIX_ALLOW_UNFREE) $(NIX_EXEC) build .#nixosConfigurations.$$host"Iso".config.system.build.isoImage $(NIX_FLAGS) --impure --show-trace; \ | ||
| iso_path=$$(find ./result -type f -name '*.iso' | head -n 1); \ |
There was a problem hiding this comment.
Using ; here allows the find and cp commands to run even if the nix build command fails. This could result in copying an ISO from a previous build (potentially for a different host) to the current host's destination. Use && to chain these commands.
$(NIX_ALLOW_UNFREE) $(NIX_EXEC) build .#nixosConfigurations.$$host"Iso".config.system.build.isoImage $(NIX_FLAGS) --impure --show-trace && \
iso_path=$$(find ./result -type f -name '*.iso' | head -n 1); \
| mkEvalCheck "nixos-matic-iso" | ||
| (import ../named-hosts/matic/iso.nix { | ||
| inherit inputs; | ||
| username = "shunkakinoki"; |
There was a problem hiding this comment.
The username used here (shunkakinoki) is inconsistent with the username defined for the maticIso host in flake.nix (skakinoki). This discrepancy means the evaluation test is not checking the actual configuration intended for this host.
username = "skakinoki";
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/modules/local-scripts/pushover-notify.sh">
<violation number="1" location="home-manager/modules/local-scripts/pushover-notify.sh:17">
P2: This fallback still loads `~/.env` when one credential is intentionally blank and the other is unset, so an explicit disable can be overridden. Only source fallback credentials when both variables are truly unset.</violation>
</file>
<file name="named-hosts/viper/default.nix">
<violation number="1" location="named-hosts/viper/default.nix:26">
P1: Do not ship this host with a hard-coded known password; it creates a trivially guessable admin login.</violation>
</file>
<file name="Makefile">
<violation number="1" location="Makefile:645">
P2: Restrict the auto-detected fallback to hosts that actually have a NixOS VM output. As written, `HOST` omitted on `galactica` resolves to a missing `nixosConfigurations.galactica` attribute and the target fails.</violation>
</file>
<file name="named-hosts/shared/linux-base.nix">
<violation number="1" location="named-hosts/shared/linux-base.nix:7">
P0: Do not ship this base module with a known default password; it creates a trivial path to root on fresh installs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| username, | ||
| hostname, | ||
| userExtraGroups ? [ ], | ||
| userInitialPassword ? "changemeow", |
There was a problem hiding this comment.
P0: Do not ship this base module with a known default password; it creates a trivial path to root on fresh installs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At named-hosts/shared/linux-base.nix, line 7:
<comment>Do not ship this base module with a known default password; it creates a trivial path to root on fresh installs.</comment>
<file context>
@@ -0,0 +1,46 @@
+ username,
+ hostname,
+ userExtraGroups ? [ ],
+ userInitialPassword ? "changemeow",
+ stateVersion ? "24.11",
+}:
</file context>
| (import ../shared/linux-base.nix { | ||
| inherit inputs pkgs username; | ||
| hostname = "viper"; | ||
| userInitialPassword = "test"; |
There was a problem hiding this comment.
P1: Do not ship this host with a hard-coded known password; it creates a trivially guessable admin login.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At named-hosts/viper/default.nix, line 26:
<comment>Do not ship this host with a hard-coded known password; it creates a trivially guessable admin login.</comment>
<file context>
@@ -0,0 +1,65 @@
+ (import ../shared/linux-base.nix {
+ inherit inputs pkgs username;
+ hostname = "viper";
+ userInitialPassword = "test";
+ })
+ (
</file context>
| if [[ -z ${PUSHOVER_API_TOKEN:-} ]] || [[ -z ${PUSHOVER_USER_KEY:-} ]]; then | ||
| # Source credentials only when the vars are unset. Explicitly blank values | ||
| # should disable notifications instead of falling back to ~/.env. | ||
| if [[ -z ${PUSHOVER_API_TOKEN+x} ]] || [[ -z ${PUSHOVER_USER_KEY+x} ]]; then |
There was a problem hiding this comment.
P2: This fallback still loads ~/.env when one credential is intentionally blank and the other is unset, so an explicit disable can be overridden. Only source fallback credentials when both variables are truly unset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/modules/local-scripts/pushover-notify.sh, line 17:
<comment>This fallback still loads `~/.env` when one credential is intentionally blank and the other is unset, so an explicit disable can be overridden. Only source fallback credentials when both variables are truly unset.</comment>
<file context>
@@ -12,8 +12,9 @@ priority="${3:-0}"
-if [[ -z ${PUSHOVER_API_TOKEN:-} ]] || [[ -z ${PUSHOVER_USER_KEY:-} ]]; then
+# Source credentials only when the vars are unset. Explicitly blank values
+# should disable notifications instead of falling back to ~/.env.
+if [[ -z ${PUSHOVER_API_TOKEN+x} ]] || [[ -z ${PUSHOVER_USER_KEY+x} ]]; then
if [[ -f "$HOME/dotfiles/.env" ]]; then
set -a
</file context>
| if [[ -z ${PUSHOVER_API_TOKEN+x} ]] || [[ -z ${PUSHOVER_USER_KEY+x} ]]; then | |
| if [[ -z ${PUSHOVER_API_TOKEN+x} ]] && [[ -z ${PUSHOVER_USER_KEY+x} ]]; then |
| build-vm: ## Build a named host VM launcher (set HOST=<name>, e.g. make build-vm HOST=viper). | ||
| @host="$(HOST)"; \ | ||
| if [ -z "$$host" ]; then \ | ||
| host="$(DETECTED_HOST)"; \ |
There was a problem hiding this comment.
P2: Restrict the auto-detected fallback to hosts that actually have a NixOS VM output. As written, HOST omitted on galactica resolves to a missing nixosConfigurations.galactica attribute and the target fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 645:
<comment>Restrict the auto-detected fallback to hosts that actually have a NixOS VM output. As written, `HOST` omitted on `galactica` resolves to a missing `nixosConfigurations.galactica` attribute and the target fails.</comment>
<file context>
@@ -638,6 +638,57 @@ nix-setup-offline: ## Set up offline environment.
+build-vm: ## Build a named host VM launcher (set HOST=<name>, e.g. make build-vm HOST=viper).
+ @host="$(HOST)"; \
+ if [ -z "$$host" ]; then \
+ host="$(DETECTED_HOST)"; \
+ fi; \
+ if [ -z "$$host" ]; then \
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
named-hosts/viper/iso.nix (1)
1-29: LGTM!The ISO configuration correctly wires all parameters through to the shared
live-iso.nixmodule. The structure properly:
- Creates a customized
pkgswith overlays and config- Passes required parameters (
inputs,pkgs,username,hostname,userInitialPassword) to the shared module- Returns a valid
nixosSystemforx86_64-linuxNote: This file is nearly identical to
matic/iso.nix. Consider extracting a sharedmkIsoConfighelper if more hosts are added, though the current duplication is acceptable for two hosts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/viper/iso.nix` around lines 1 - 29, Extract the duplicated ISO wiring into a shared helper (e.g., mkIsoConfig) and replace the near-identical bodies in viper/iso.nix and matic/iso.nix with calls to that helper; the helper should accept parameters used in the diff (inputs, username, system, overlays/nixpkgsConfig or a prebuilt pkgs) and invoke import ../shared/live-iso.nix with inherit inputs pkgs username and allow passing hostname and userInitialPassword, so both files simply construct any host-specific values then call mkIsoConfig(hostname, userInitialPassword, username, inputs).named-hosts/viper/default.nix (1)
54-61: RedundantinitialPasswordoverride invmVariant.Line 26 already passes
userInitialPassword = "test"tolinux-base.nix, which setsusers.users.${username}.initialPassword. Thelib.mkForce "test"on line 60 is redundant unless the intent is to guard against future changes to the base module's default.If intentional as a safeguard, consider adding a brief comment. Otherwise, the duplicate can be removed.
🔧 Proposed simplification (if not needed as safeguard)
virtualisation.vmVariant = { virtualisation = { graphics = false; memorySize = 4096; cores = 4; }; - users.users.${username}.initialPassword = lib.mkForce "test"; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/viper/default.nix` around lines 54 - 61, The users.users.${username}.initialPassword override inside virtualisation.vmVariant currently forces "test" with lib.mkForce while the same value is already passed as userInitialPassword = "test" into linux-base.nix; remove the redundant users.users.${username}.initialPassword = lib.mkForce "test" line from virtualisation.vmVariant unless you intentionally want a safeguard against base-module changes—if it is intentional, replace the override with a short comment explaining that it is a deliberate hard-force protection and reference userInitialPassword and linux-base.nix so future readers understand why both exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 137-148: The usernames are inconsistent: flake.nix sets matic and
maticIso to username = "skakinoki" while viper and viperIso use "shunkakinoki",
and tests/eval.nix evaluates the module with "shunkakinoki"; decide which is
correct and make them consistent — either change matic and maticIso in flake.nix
(symbols: matic, maticIso) to username = "shunkakinoki" to match tests and
viper, or update tests/eval.nix to use "skakinoki" if the split is intentional;
ensure the same username value is used wherever the same host/module is
evaluated so the test exercises the deployed configuration.
---
Nitpick comments:
In `@named-hosts/viper/default.nix`:
- Around line 54-61: The users.users.${username}.initialPassword override inside
virtualisation.vmVariant currently forces "test" with lib.mkForce while the same
value is already passed as userInitialPassword = "test" into linux-base.nix;
remove the redundant users.users.${username}.initialPassword = lib.mkForce
"test" line from virtualisation.vmVariant unless you intentionally want a
safeguard against base-module changes—if it is intentional, replace the override
with a short comment explaining that it is a deliberate hard-force protection
and reference userInitialPassword and linux-base.nix so future readers
understand why both exist.
In `@named-hosts/viper/iso.nix`:
- Around line 1-29: Extract the duplicated ISO wiring into a shared helper
(e.g., mkIsoConfig) and replace the near-identical bodies in viper/iso.nix and
matic/iso.nix with calls to that helper; the helper should accept parameters
used in the diff (inputs, username, system, overlays/nixpkgsConfig or a prebuilt
pkgs) and invoke import ../shared/live-iso.nix with inherit inputs pkgs username
and allow passing hostname and userInitialPassword, so both files simply
construct any host-specific values then call mkIsoConfig(hostname,
userInitialPassword, username, inputs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4b8e354-5e08-46dc-8db6-9b69fa12ad99
📒 Files selected for processing (12)
.gitignoreMakefileflake.nixhome-manager/modules/local-scripts/clipboard-copy.shhome-manager/modules/local-scripts/pushover-notify.shnamed-hosts/matic/default.nixnamed-hosts/matic/iso.nixnamed-hosts/shared/linux-base.nixnamed-hosts/shared/live-iso.nixnamed-hosts/viper/default.nixnamed-hosts/viper/iso.nixtests/eval.nix
| maticIso = import ./named-hosts/matic/iso.nix { | ||
| inherit inputs; | ||
| username = "skakinoki"; | ||
| }; | ||
| viper = import ./named-hosts/viper { | ||
| inherit inputs; | ||
| username = "shunkakinoki"; | ||
| }; | ||
| viperIso = import ./named-hosts/viper/iso.nix { | ||
| inherit inputs; | ||
| username = "shunkakinoki"; | ||
| }; |
There was a problem hiding this comment.
Username inconsistency between flake.nix and tests/eval.nix.
The maticIso configuration uses username = "skakinoki" here, but tests/eval.nix evaluates the same module with username = "shunkakinoki" (line 81). While the evaluation check will still pass, this means the test doesn't exercise the exact configuration that will be deployed.
Also note the username difference between hosts:
matic/maticIso:"skakinoki"viper/viperIso:"shunkakinoki"
If this is intentional (different hosts for different users), no action needed. If "skakinoki" is a typo for "shunkakinoki", consider aligning them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 137 - 148, The usernames are inconsistent: flake.nix
sets matic and maticIso to username = "skakinoki" while viper and viperIso use
"shunkakinoki", and tests/eval.nix evaluates the module with "shunkakinoki";
decide which is correct and make them consistent — either change matic and
maticIso in flake.nix (symbols: matic, maticIso) to username = "shunkakinoki" to
match tests and viper, or update tests/eval.nix to use "skakinoki" if the split
is intentional; ensure the same username value is used wherever the same
host/module is evaluated so the test exercises the deployed configuration.
1 participant
Summary by cubic
Add live/installer ISO and VM workflows for named hosts and introduce the
viperhost. Shared base modules reduce duplication, and new Makefile targets make it easy to build and run VMs or ISOs.New Features
build-vm,run-vm, andbuild-iso(copies to./<host>.iso).named-hosts/shared/linux-base.nixandlive-iso.nixfor reusable host/ISO config.viperwith QEMU guest profile, GRUB on/dev/vda, and VM settings; ISO variant included.maticIso,viper, andviperIso; tests added for VM and ISO builds.Refactors
matichost now importslinux-base.nixto remove duplicated user/network/nix settings.wl-copy, then falls back topbcopyandxclip..envwhen vars are unset; blank vars disable notifications; network errors don’t fail the script..gitignorenow ignores*.iso.Written for commit 58a4547. Summary will update on new commits.