fix regression when installing on systems with dash #591
Conversation
WalkthroughUpdates kexec flow in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant L as Local (nixos-anywhere.sh)
participant R as Remote host
participant S as Remote script (unpack.sh)
participant K as kexec/installer
participant LOG as remote log
Note over L: prepare kexec tarball and resolve remoteHomeDir
L->>R: scp tarball -> remoteHomeDir
L->>R: ssh write unpack.sh into remoteHomeDir
L->>R: ssh chmod +x unpack.sh
rect rgb(240,250,240)
Note over R,S: consolidated remote execution (dirs, extract, run)
L->>R: ssh execute ./unpack.sh
S->>LOG: tee all output to remote log
S->>K: run kexec installer
K-->>S: exit status + output
end
alt Expected success marker present
S-->>R: exit 0 and success marker in LOG
R-->>L: ssh exit 0
else Missing marker or non-zero exit
S-->>R: non-zero or missing marker
R-->>L: ssh exit non-zero
L->>R: scp/fetch remote LOG
L-->>L: handleKexecFailure(operation) -> exit non-zero
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Mic92
force-pushed
the
fix_password_prompt
branch
from
167fd42 to
2269a1d
Compare
Mic92
force-pushed
the
fix_password_prompt
branch
from
2269a1d to
086921a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nixos-anywhere.sh (1)
752-771: Critical:Line 763 uses
print '%q' "$kexecExtraFlags"which is not available in dash or bash. The PR title mentions fixing a regression on systems with dash, but this would fail on such systems. Useprintfinstead.Apply this diff:
- TMPDIR=\"$remoteHomeDir/kexec\" ${maybeSudo} setsid --wait \"$remoteHomeDir/kexec/kexec/run\" --kexec-extra-flags $(print '%q' "$kexecExtraFlags") + TMPDIR=\"$remoteHomeDir/kexec\" ${maybeSudo} setsid --wait \"$remoteHomeDir/kexec/kexec/run\" --kexec-extra-flags $(printf '%q' "$kexecExtraFlags")
🧹 Nitpick comments (2)
src/nixos-anywhere.sh (2)
746-749: Redundant validation check.The
remoteHomeDirvariable is already validated inimportFacts()at lines 589-593, which is called beforerunKexec()in the execution flow. This check at line 747 will never trigger because the script would have already aborted earlier ifremoteHomeDirwere empty.Consider removing this redundant check:
- # Extract directly to the user's home directory - if [[ -z $remoteHomeDir ]]; then - abort "Could not determine home directory for user $sshUser" - fi -
800-810: Consider cleanup of uploaded tarball.When uploading the kexec tarball to
$remoteHomeDir/kexec-tarball.tar.gz(line 807), the file is left on the remote system after extraction. While not critical, adding cleanup would save disk space.Consider adding cleanup to the remote script template by appending to line 762:
# Execute tar command %TAR_COMMAND% + rm -f \"$remoteHomeDir\"/kexec-tarball.tar.gz TMPDIR=\"$remoteHomeDir/kexec\" ${maybeSudo} setsid --wait \"$remoteHomeDir/kexec/kexec/run\" --kexec-extra-flags $(printf '%q' "$kexecExtraFlags")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nixos-anywhere.sh(2 hunks)
🔇 Additional comments (2)
src/nixos-anywhere.sh (2)
729-744: LGTM! Cleaner failure handling.The refactored function simplifies the signature by removing the exit code parameter and always treating calls as failures. The log retrieval with debug suppression is well done.
812-819: LGTM! Clean script generation and execution.The approach of generating a complete script remotely and then executing it improves robustness and makes debugging easier. The error handling with
handleKexecFailureis appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/nixos-anywhere.sh (2)
746-749: Clarify error message variable.The error message references
$sshUser, but this variable represents the SSH connection user. Consider whether this is the most appropriate variable name in this context, or if a more descriptive message would be clearer.Apply this diff to improve clarity:
- if [[ -z $remoteHomeDir ]]; then - abort "Could not determine home directory for user $sshUser" - fi + if [[ -z $remoteHomeDir ]]; then + abort "Could not determine remote home directory" + fi
800-810: Review tar command construction and upload logic.The logic correctly handles both local and remote download scenarios:
- Remote download: constructs a command that downloads and extracts in one pipe
- Local upload: uploads the tarball first, then extracts from the uploaded file
One minor observation: when uploading locally (line 807), the file is always named
kexec-tarball.tar.gzregardless of the actual compression format. While this doesn't affect functionality (since thetarDecompflag explicitly specifies the format), it could be slightly confusing.Consider using a more accurate filename:
- "${localUploadCommand[@]}" | runSsh "cat > \"$remoteHomeDir\"/kexec-tarball.tar.gz" + local tarballName="kexec-tarball.tar${kexecUrl##*.tar}" + "${localUploadCommand[@]}" | runSsh "cat > \"$remoteHomeDir\"/\"$tarballName\"" # Use local file for extraction - tarCommand="cat \"$remoteHomeDir\"/kexec-tarball.tar.gz | tar -xv ${tarDecomp}" + tarCommand="cat \"$remoteHomeDir\"/\"$tarballName\" | tar -xv ${tarDecomp}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nixos-anywhere.sh(2 hunks)
🔇 Additional comments (3)
src/nixos-anywhere.sh (3)
729-744: LGTM!The simplified
handleKexecFailurefunction correctly attempts to fetch and display the remote log before exiting. The error suppression when fetching the log is appropriate for handling cases where the log file doesn't exist.
754-771: Verify the remote command template logic.The template wraps kexec execution in a subshell with error handling. A few observations:
- Line 764 uses
|| trueto prevent the script from exiting immediately, allowing the log to be captured even on failure.- Lines 767-770 check for the absence of the success message and treat it as failure - this inverts the typical success-checking pattern.
While this approach should work, ensure that the success message "machine will boot into nixos" is reliably written to the log before the kexec disconnects the SSH session.
816-819: LGTM! Bash execution ensures compatibility.The script creation and execution correctly:
- Creates the remote
unpack.shscript via heredoc- Executes it explicitly with
bash(notsh), ensuring bash-specific features in the template work correctly- Handles failures by calling
handleKexecFailureThis approach addresses the regression mentioned in the PR title by ensuring bash (not dash or another POSIX shell) interprets the script.
Summary by CodeRabbit
Refactor
Bug Fixes