refactor(log): generalize interface and add new log types#111
refactor(log): generalize interface and add new log types#111water-sucks merged 5 commits intonix-community:mainfrom
Conversation
WalkthroughReplaces the concrete Logger with a Logger interface and adds ConsoleLogger, SyslogLogger, and MultiLogger. Converts many logger parameters from pointer to value, removes explicit verbose plumbing in favor of log-level-driven behavior, and updates many commands to initialize/inject contextual loggers via PreRun hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User CLI
participant Pre as PreRun
participant Ctx as Context
participant Log as Logger (Console/Syslog/Multi)
participant Cmd as Command Handler
Note over CLI,Cmd: New flow: log-level driven, PreRun injects logger
CLI->>Pre: invoke PreRun (init logger from context)
Pre->>Ctx: FromContext()
alt opts.Verbose
Pre->>Log: SetLogLevel(Debug)
end
Pre->>Ctx: WithLogger(updated logger)
CLI->>Cmd: RunE / main handler
Cmd->>Log: Debug/Info/Warn/Error (calls routed to MultiLogger if present)
Cmd->>Log: CmdArray(argv) (logger decides -v and output)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: Wide-reaching API surface changes (logger interface, multiple implementations, pointer→value migration, removal of verbose plumbing) across many packages require careful review of interface contracts, context propagation, and logging semantics; however changes are systematic. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
cmd/generation/rollback/rollback.go (1)
184-205: Avoid panicking when current generation isn’t found. Return an error instead.Panics here can crash the CLI (and shell completion). Prefer an error path.
Apply this diff:
func findPreviousGeneration(log logger.Logger, profileName string) (*generation.Generation, error) { @@ - if currentGenIdx == -1 { - panic("current generation not found, this is a bug") - } + if currentGenIdx == -1 { + msg := "current generation not found" + log.Error(msg) + return nil, fmt.Errorf("%s", msg) + }internal/system/local.go (1)
17-21: Add a constructor guard for nil logger.A nil logger (nil interface) will panic at first method call elsewhere. Fail fast here.
Apply this diff:
func NewLocalSystem(logger logger.Logger) *LocalSystem { - return &LocalSystem{ + if logger == nil { + panic("nil logger passed to NewLocalSystem") + } + return &LocalSystem{ logger: logger, } }cmd/init/generate.go (2)
53-57: Duplicate appends to extraAttrs will emit duplicate attributesextraAttrs is populated twice, likely generating duplicate keys in the rendered Nix attrset.
Remove the second append block:
- if cfg.Init.ExtraAttrs != nil { - for k, v := range cfg.Init.ExtraAttrs { - extraAttrs = append(extraAttrs, KVPair{Key: k, Value: v}) - } - }
123-125: Use correct kernel module name "bcachefs" instead of "bcache"The kernel module is named "bcachefs" (module file bcachefs.ko). The code currently appends "bcache" to the modules list, which is incorrect and will fail to load the bcachefs filesystem support. Update line 124 to append "bcachefs" instead.
cmd/activate/run.go (1)
117-121: Wrong variable in error messageFormats the list of specialisations instead of the requested name.
Apply:
- err = fmt.Errorf("specialisation '%v' does not exist", specialisations) + err = fmt.Errorf("specialisation '%v' does not exist", specialisation)cmd/init/filesystems.go (2)
37-68: Harden swap parsing against malformed linesBounds-check fields to avoid OOB panics if /proc/swaps has unexpected content.
_ = s.Scan() // Skip header line for s.Scan() { fields := strings.Fields(s.Text()) - swapFilename := fields[0] - swapType := fields[1] + if len(fields) < 2 { + log.Warnf("malformed swap entry: %q", s.Text()) + continue + } + swapFilename := fields[0] + swapType := fields[1]
70-220: Prevent OOBs and fix space escaping in mountinfo parsing
- Several indexes (fields[2], [3], [4], [5]) are read before validating length — risk of panic on malformed lines.
- ReplaceAll("\040", "") removes spaces; should decode to " " for paths.
- Minor: error message says “swap device list” when opening mountinfo.
- Optional: Trim newline from loop backer file.
mountList, err := os.Open(mountedFilesystemListFilename) if err != nil { - log.Warnf("failed to open swap device list %v: %v", mountedFilesystemListFilename, err) + log.Warnf("failed to open mounted filesystem list %v: %v", mountedFilesystemListFilename, err) return filesystems } @@ for s.Scan() { fields := strings.Fields(s.Text()) - mountID := fields[2] - path := fields[3] + if len(fields) < 6 { + log.Warnf("malformed mount entry: %q", s.Text()) + continue + } + mountID := fields[2] + path := fields[3] if path == "/" { path = "" } - absoluteMountpoint := strings.ReplaceAll(fields[4], "\\040", "") + absoluteMountpoint := strings.ReplaceAll(fields[4], "\\040", " ") @@ - mountOptions := strings.Split(fields[5], ",") + mountOptions := strings.Split(fields[5], ",") @@ - devicePath := fields[n+1] - devicePath = strings.ReplaceAll(devicePath, "\\040", "") + devicePath := fields[n+1] + devicePath = strings.ReplaceAll(devicePath, "\\040", " ") devicePath = strings.ReplaceAll(devicePath, "\\011", "\t") @@ - if backer, err := os.ReadFile(backerFilename); err == nil { - devicePath = string(backer) + if backer, err := os.ReadFile(backerFilename); err == nil { + devicePath = strings.TrimSpace(string(backer)) extraOptions = append(extraOptions, "loop") }Optional: consider using a parser for /proc/self/mountinfo (e.g., github.com/moby/sys/mountinfo) to avoid manual field handling.
cmd/enter/run.go (2)
111-114: Fix Warnf: missing format verb and wrong error variableUse the collected resolvConfErr and include it in the message.
-resolvConfDone: - if resolvConfErr != nil { - log.Warnf("Internet access may not be available", err) - } +resolvConfDone: + if resolvConfErr != nil { + log.Warnf("Internet access may not be available: %v", resolvConfErr) + }
133-135: Correct the condition checking both command sourceslen(opts.Command) > 1 misses 1‑char commands. Check non‑empty instead.
- if len(opts.CommandArray) > 0 && len(opts.Command) > 1 { + if len(opts.CommandArray) > 0 && opts.Command != "" {cmd/generation/list/tui.go (1)
11-20: Fix import aliases to match identifiers used (build breaker)This file references cmdUtils and genUtils but imports without aliases, causing compile errors.
- "github.com/nix-community/nixos-cli/cmd/generation/shared" - "github.com/nix-community/nixos-cli/internal/cmd/utils" + genUtils "github.com/nix-community/nixos-cli/cmd/generation/shared" + cmdUtils "github.com/nix-community/nixos-cli/internal/cmd/utils"Also applies to: 297-335
cmd/install/install.go (2)
135-179: Fix directory permission walk: SplitList is wrong for path componentsfilepath.SplitList splits PATH‑like lists, not path components. Use path separator split to walk each directory.
- for _, component := filepath.SplitList(mountpoint) { + for _, component := strings.Split(mountpoint, string(os.PathSeparator)) { if component == "" { continue } currentPath = filepath.Join(currentPath, component) @@ - log.Printf("hint: consider running `chmod o+rx %s", currentPath) + log.Printf("hint: consider running `chmod o+rx %s`", currentPath)
467-479: Fix manual hint: placeholder and wordingUse the actual mountpoint and fix the grammar.
- manualHint := "you can set the root password manually by executing `nixos enter --root {s}` and then running `passwd` in the shell of them new system" + manualHint := fmt.Sprintf("you can set the root password manually by executing `nixos enter --root %s` and then running `passwd` in the shell of the new system", mountpoint)internal/activation/activation.go (1)
62-79: Fix inverted system-profile check only; exit code handling is already correctThe system profile directory check is indeed inverted and should be
profile == "system". However, the exit code handling suggestion is unnecessary—theCommandRunner.Run()method already returns an error when a command exits with a non-zero code viaexec.ExitError, so the current pattern of ignoring the exit code and returning only the error is correct.Apply only:
func AddNewNixProfile(s system.CommandRunner, profile string, closure string) error { - if profile != "system" { + if profile == "system" { err := EnsureSystemProfileDirectoryExists() if err != nil { return err } }The same inverted condition exists in
SetNixProfileGeneration()at line 82 and should also be corrected.
🧹 Nitpick comments (16)
cmd/option/completion.go (1)
25-36: Downgrade cache-miss to debug; warn only on real I/O errorsAvoid warning on an expected cache-miss; check os.ErrNotExist explicitly.
@@ - _, err = os.Stat(prebuiltOptionCachePath) - if err != nil { - log.Warnf("error accessing prebuilt option cache: %v", err) - useCache = false - } + _, err = os.Stat(prebuiltOptionCachePath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + log.Debugf("prebuilt option cache not found at %s", prebuiltOptionCachePath) + } else { + log.Warnf("error accessing prebuilt option cache %s: %v", prebuiltOptionCachePath, err) + } + useCache = false + }Note: add
errorsto imports.internal/generation/generation.go (1)
149-171: Avoid inner shadowing; compute base dir onceThe inner
profileDirectory := ...later in the loop shadows the outer variable. Compute once and reuse to reduce confusion.@@ func CollectGenerationsInProfile(log logger.Logger, profile string) ([]Generation, error) { - profileDirectory := constants.NixProfileDirectory + profileDirectory := constants.NixProfileDirectory if profile != "system" { profileDirectory = constants.NixSystemProfileDirectory } @@ - profileDirectory := constants.NixProfileDirectory - if profile != "system" { - profileDirectory = constants.NixSystemProfileDirectory - } - - generationDirectoryName := filepath.Join(profileDirectory, fmt.Sprintf("%s-%d-link", profile, genNumber)) + generationDirectoryName := filepath.Join(profileDirectory, fmt.Sprintf("%s-%d-link", profile, genNumber))internal/configuration/flake.go (1)
136-139: Consider threshold semantics for verbosityUsing exact equality may miss future “more verbose than debug” levels. Prefer a helper like
AtLeast(LogLevelDebug)or a comparison that reflects your level ordering.cmd/generation/shared/utils.go (1)
23-27: Use slices.Reverse for clarity.Replace the manual swap loop with slices.Reverse for readability (Go 1.21+; already used elsewhere).
Apply this diff:
+import "slices" @@ - if reverse { - for i, j := 0, len(generations)-1; i < j; i, j = i+1, j-1 { - generations[i], generations[j] = generations[j], generations[i] - } - } + if reverse { + slices.Reverse(generations) + }cmd/generation/rollback/rollback.go (1)
207-226: Completion should tolerate missing logger.logger.FromContext() can panic in completion contexts. Consider a safe retrieval or default console logger for completion.
If a safe accessor exists (e.g., FromContextOrDefault), switch to it; otherwise, I can draft one.
internal/system/local.go (1)
23-48: Consider a context-aware runner to support cancelation/timeouts.exec.Command lacks cancelation; a
RunContext(ctx, *Command)variant using exec.CommandContext would improve UX and robustness.I can draft a minimal
RunContextmethod and thread contexts from commands if you want.internal/logger/context.go (2)
9-11: Guard against nil logger in context injectionDefensive nil check avoids storing a nil that later panics on retrieval.
Apply:
func WithLogger(ctx context.Context, logger Logger) context.Context { + if logger == nil { + panic("nil logger passed to WithLogger") + } return context.WithValue(ctx, loggerCtxKey, logger) }
13-20: Provide a non‑panicking retrieval helperMany call sites benefit from an ok-returning variant.
Add:
func FromContext(ctx context.Context) Logger { logger, ok := ctx.Value(loggerCtxKey).(Logger) if !ok { panic("logger not present in context") } return logger } + +// FromContextOk returns the logger and whether it was present without panicking. +func FromContextOk(ctx context.Context) (Logger, bool) { + l, ok := ctx.Value(loggerCtxKey).(Logger) + return l, ok && l != nil +}cmd/init/generate.go (1)
296-313: nixStringList leaves empty entries; simplify and dedupe cleanlyCurrent preallocation + continue leaves empty strings joined by spaces. Prefer append on unique items.
Apply:
func nixStringList(s []string) string { - itemSet := make(map[string]bool) - quotedItems := make([]string, len(s)) - - for i, item := range s { - if itemSet[item] { - continue - } - - itemSet[item] = true - quotedItems[i] = nixString(item) - } - - return strings.Join(quotedItems, " ") + seen := make(map[string]struct{}, len(s)) + out := make([]string, 0, len(s)) + for _, item := range s { + if _, ok := seen[item]; ok { + continue + } + seen[item] = struct{}{} + out = append(out, nixString(item)) + } + return strings.Join(out, " ") }cmd/activate/run.go (1)
314-318: Initialize syslog before constructing the system runner; propagate to context and sync levelsCreate the MultiLogger first, update the command context, then build the CommandRunner so s.Logger() includes syslog. Also align syslog’s level with the current logger.
Apply:
- log := logger.FromContext(cmd.Context()) - s := system.NewLocalSystem(log) + log := logger.FromContext(cmd.Context()) @@ - if syslogLogger, err := logger.NewSyslogLogger("nixos-cli-activate"); err == nil { - log = logger.NewMultiLogger(log, syslogLogger) - } else { - log.Warnf("failed to initialize syslog logger: %v", err) - } + if syslogLogger, err := logger.NewSyslogLogger("nixos-cli-activate"); err == nil { + syslogLogger.SetLogLevel(log.GetLogLevel()) + log = logger.NewMultiLogger(log, syslogLogger) + } else { + log.Warnf("failed to initialize syslog logger: %v", err) + } + // Ensure downstream lookups get the combined logger. + cmd.SetContext(logger.WithLogger(cmd.Context(), log)) + // Construct runner with the final logger. + s := system.NewLocalSystem(log)Also applies to: 385-389
internal/generation/diff.go (1)
14-15: Use the local logger variable consistentlyMinor cleanup; avoids redundant s.Logger() call.
Apply:
- log := s.Logger() + log := s.Logger() @@ - s.Logger().CmdArray(argv) + log.CmdArray(argv)Also applies to: 33-34
cmd/init/filesystems.go (1)
222-241: Prefer structured parsing for LVM detectionThe lsblk substring check can false‑positive. Consider lsblk JSON and checking types.
- cmd := system.NewCommand("lsblk", "-o", "TYPE") + cmd := system.NewCommand("lsblk", "-J", "-o", "TYPE")Then parse stdout JSON and look for any block with "type":"lvm".
cmd/enter/run.go (1)
45-46: Polish debug messageGrammar nit.
- log.Debug("sandboxed process successfully") + log.Debug("sandboxed process initialized successfully")cmd/generation/delete/delete.go (1)
257-281: GC verbosity toggling is fine; consider >= checkIf more verbose levels are added later, using >= debug (or a predicate) may age better than equality.
- if log.GetLogLevel() == logger.LogLevelDebug { + if log.GetLogLevel() <= logger.LogLevelDebug { argv = append(argv, "-v") }internal/activation/activation.go (1)
184-190: Place “-v” before the action and avoid duplicate Logger() callMany CLIs expect flags before positional args. Safer to insert “-v” after the command path, then the action. Also reuse the local logger var.
- log := s.Logger() - if log.GetLogLevel() == logger.LogLevelDebug { - argv = append(argv, "-v") - } - - s.Logger().CmdArray(argv) + log := s.Logger() + if log.GetLogLevel() == logger.LogLevelDebug { + argv = append([]string{argv[0], "-v"}, argv[1:]...) + } + log.CmdArray(argv)Please verify switch-to-configuration accepts “-v” in this position. If it requires trailing flags, keep the original order.
internal/logger/syslog.go (1)
113-119: Use Debug severity for CmdArray to match debug-only gatingCurrently gated by debug but logged at Info.
func (l *SyslogLogger) CmdArray(argv []string) { if l.level > LogLevelDebug { return } - _ = l.writer.Info(fmt.Sprintf("$ %v", utils.EscapeAndJoinArgs(argv))) + _ = l.writer.Debug(fmt.Sprintf("$ %v", utils.EscapeAndJoinArgs(argv))) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
cmd/activate/run.go(2 hunks)cmd/activate/user.go(1 hunks)cmd/apply/apply.go(9 hunks)cmd/enter/enter.go(2 hunks)cmd/enter/run.go(8 hunks)cmd/generation/delete/delete.go(3 hunks)cmd/generation/diff/diff.go(2 hunks)cmd/generation/list/tui.go(3 hunks)cmd/generation/rollback/rollback.go(5 hunks)cmd/generation/shared/utils.go(1 hunks)cmd/generation/switch/switch.go(4 hunks)cmd/init/cpuinfo.go(2 hunks)cmd/init/devices.go(3 hunks)cmd/init/filesystems.go(4 hunks)cmd/init/generate.go(3 hunks)cmd/init/run.go(1 hunks)cmd/install/install.go(15 hunks)cmd/option/completion.go(1 hunks)cmd/option/option.go(1 hunks)cmd/repl/repl.go(1 hunks)cmd/root/root.go(1 hunks)internal/activation/activation.go(5 hunks)internal/configuration/configuration.go(2 hunks)internal/configuration/flake.go(3 hunks)internal/configuration/legacy.go(3 hunks)internal/generation/diff.go(2 hunks)internal/generation/generation.go(1 hunks)internal/generation/specialisations.go(1 hunks)internal/logger/console.go(1 hunks)internal/logger/context.go(1 hunks)internal/logger/logger.go(1 hunks)internal/logger/multi.go(1 hunks)internal/logger/syslog.go(1 hunks)internal/system/local.go(2 hunks)internal/system/runner.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (33)
cmd/activate/user.go (1)
internal/logger/logger.go (1)
Logger(12-27)
cmd/generation/shared/utils.go (2)
internal/logger/logger.go (1)
Logger(12-27)internal/generation/generation.go (1)
Generation(27-38)
cmd/init/cpuinfo.go (2)
internal/logger/logger.go (1)
Logger(12-27)internal/system/runner.go (1)
CommandRunner(10-13)
internal/logger/context.go (1)
internal/logger/logger.go (1)
Logger(12-27)
cmd/generation/list/tui.go (1)
internal/logger/logger.go (1)
Logger(12-27)
internal/generation/generation.go (1)
internal/logger/logger.go (1)
Logger(12-27)
internal/configuration/legacy.go (1)
internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)
cmd/enter/run.go (2)
internal/logger/logger.go (1)
Logger(12-27)internal/system/runner.go (1)
CommandRunner(10-13)
cmd/generation/delete/delete.go (5)
internal/system/runner.go (3)
Command(15-22)CommandRunner(10-13)NewCommand(24-33)internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/constants/constants.go (1)
CurrentSystem(7-7)internal/build/build.go (1)
Flake(32-34)
cmd/generation/rollback/rollback.go (5)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/constants/constants.go (1)
CurrentSystem(7-7)internal/activation/activation.go (1)
SetNixProfileGeneration(81-98)
internal/logger/console.go (2)
internal/logger/logger.go (6)
Logger(12-27)LogLevel(3-3)LogLevelInfo(7-7)LogLevelDebug(6-6)LogLevelWarn(8-8)LogLevelError(9-9)internal/utils/utils.go (1)
EscapeAndJoinArgs(27-41)
cmd/generation/diff/diff.go (3)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (1)
LogLevelDebug(6-6)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)
internal/system/local.go (1)
internal/logger/logger.go (1)
Logger(12-27)
internal/generation/diff.go (2)
internal/system/runner.go (1)
CommandRunner(10-13)internal/logger/logger.go (1)
Logger(12-27)
cmd/activate/run.go (3)
internal/logger/logger.go (1)
Logger(12-27)internal/logger/syslog.go (1)
NewSyslogLogger(19-31)internal/logger/multi.go (1)
NewMultiLogger(8-10)
cmd/init/devices.go (1)
internal/logger/logger.go (1)
Logger(12-27)
cmd/repl/repl.go (1)
internal/configuration/configuration.go (1)
FindConfiguration(43-69)
internal/logger/multi.go (1)
internal/logger/logger.go (2)
Logger(12-27)LogLevel(3-3)
internal/generation/specialisations.go (1)
internal/configuration/configuration.go (1)
FindConfiguration(43-69)
cmd/init/generate.go (5)
internal/system/runner.go (1)
CommandRunner(10-13)internal/settings/settings.go (1)
Settings(15-27)cmd/init/cpuinfo.go (1)
VirtualisationType(74-74)internal/cmd/opts/opts.go (1)
InitOpts(137-143)internal/logger/logger.go (1)
Logger(12-27)
cmd/enter/enter.go (2)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (1)
LogLevelDebug(6-6)
cmd/option/completion.go (3)
internal/logger/logger.go (1)
Logger(12-27)internal/system/local.go (1)
NewLocalSystem(17-21)internal/configuration/configuration.go (1)
FindConfiguration(43-69)
internal/logger/syslog.go (2)
internal/logger/logger.go (5)
LogLevel(3-3)LogLevelInfo(7-7)LogLevelDebug(6-6)LogLevelWarn(8-8)LogLevelError(9-9)internal/utils/utils.go (1)
EscapeAndJoinArgs(27-41)
internal/configuration/configuration.go (4)
internal/logger/logger.go (1)
Logger(12-27)internal/settings/settings.go (1)
Settings(15-27)internal/build/build.go (1)
Flake(32-34)internal/configuration/legacy.go (1)
FindLegacyConfiguration(24-82)
cmd/install/install.go (3)
internal/system/runner.go (2)
Command(15-22)CommandRunner(10-13)internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)
cmd/option/option.go (1)
internal/configuration/configuration.go (1)
FindConfiguration(43-69)
cmd/init/filesystems.go (2)
internal/logger/logger.go (1)
Logger(12-27)internal/system/runner.go (1)
CommandRunner(10-13)
cmd/apply/apply.go (5)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/configuration/configuration.go (1)
FindConfiguration(43-69)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/activation/activation.go (2)
AddNewNixProfile(62-79)SetNixProfileGeneration(81-98)
internal/system/runner.go (1)
internal/logger/logger.go (1)
Logger(12-27)
internal/configuration/flake.go (1)
internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)
internal/activation/activation.go (2)
internal/system/runner.go (2)
CommandRunner(10-13)NewCommand(24-33)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)
cmd/generation/switch/switch.go (4)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (1)
LogLevelDebug(6-6)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/activation/activation.go (1)
SetNixProfileGeneration(81-98)
cmd/root/root.go (1)
internal/logger/console.go (1)
NewConsoleLogger(30-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (41)
internal/generation/specialisations.go (1)
99-99: LGTM!The call to
FindConfigurationcorrectly aligns with the updated signature, removing the verbose boolean parameter and passing the logger by value.cmd/activate/user.go (1)
41-41: LGTM!The function signature correctly updated to accept
logger.Loggerby value instead of a pointer, aligning with the new logger interface design.cmd/root/root.go (1)
58-58: LGTM!The change from
logger.NewLogger()tologger.NewConsoleLogger()correctly initializes the new ConsoleLogger implementation, which provides colorized, level-based output as part of the broader logging refactor.cmd/init/devices.go (3)
48-48: LGTM!The function signature correctly updated to accept
logger.Loggerby value instead of a pointer, consistent with the logger interface refactor.
123-123: LGTM!The function signature correctly updated to accept
logger.Loggerby value instead of a pointer, consistent with the logger interface refactor.
151-151: LGTM!The function signature correctly updated to accept
logger.Loggerby value instead of a pointer, consistent with the logger interface refactor.cmd/option/option.go (1)
111-111: LGTM!The call to
FindConfigurationcorrectly aligns with the updated signature, removing the verbose boolean parameter and passing the logger by value.cmd/enter/enter.go (1)
18-35: LGTM!The command correctly adopts the new logging initialization pattern:
- Separates argument validation into the
Argsfunction.- Adds a
PreRunhook to configure the logger from context, apply verbose debug level when requested, and propagate the configured logger back to the command context for use inRunE.This pattern aligns with the broader logger refactor across the codebase.
cmd/init/run.go (2)
23-23: Verification successful—no issues found.The function
determineVirtualisationTypecorrectly obtains the logger internally vias.Logger()on line 112 ofcmd/init/cpuinfo.goand uses it for warning messages (lines 128 and 148). The removal of the logger parameter from the call incmd/init/run.go:23does not cause any issues.
27-27: No changes required — implementation correctly obtains logger internally.The verification confirms that
generateHwConfigNixat line 31 of cmd/init/generate.go obtains the logger vialog := s.Logger()and uses it throughout the function and in all dependent function calls. The parameter signature change is handled correctly.internal/system/runner.go (1)
12-12: AllCommandRunnerimplementations correctly returnlogger.Loggerby value.Verification confirms:
- The
CommandRunnerinterface atinternal/system/runner.go:10-13correctly specifiesLogger() logger.Logger(value return)- The only concrete implementation (
LocalSystematinternal/system/local.go:71-73) correctly implementsLogger() logger.Loggerand returnsl.loggerby value- No implementations with the old pointer signature remain
The interface change has been properly implemented across the codebase.
cmd/option/completion.go (2)
16-20: Signature and call-site change LGTMSwitch to logger.Logger (value) and updated FindConfiguration call are consistent with the new interface-driven logging. No functional regressions spotted.
60-73: No issues found—logger is properly propagated to completion contextThe root command explicitly calls
cmd.SetContext(cmdCtx)at line 110 ofcmd/root/root.go, injecting the logger before any command execution. The option command is a subcommand that inherits this context via Cobra's context propagation mechanism, so whenOptionsCompletionFunccallslogger.FromContext(cmd.Context()), it receives the initialized context with the logger already present. No panic occurs.cmd/init/cpuinfo.go (2)
39-52: LGTM: logger passed by valueSignature change aligns with the new logger interface; behavior unchanged.
111-151: LGTM: derive logger from runnerUsing s.Logger() removes the need to thread log explicitly and keeps concerns local. No functional issues found.
cmd/generation/diff/diff.go (3)
62-62: Flag wiring LGTM
--verbose/-vtoggles debug via PreRun; consistent with new log-level approach.
87-89: Updated RunDiffCommand usage LGTMCall site now derives logging via CommandRunner; matches internal/generation API.
46-56: Redundancy confirmed, but proposed refactor needs careful reviewThe logger is confirmed present in context before PreRun executes (via
cmd/root/root.go:59). SetLogLevel mutates the logger in-place (internal/logger/console.go:45,internal/logger/syslog.go:34), making the re-injection redundant.However, the proposed diff removes not just the redundant
logger.WithLogger()but alsocmd.SetContext()entirely. This is a behavioral change: while the logger mutation persists due to in-place modification, explicitly updating the command's context is removed. The pattern is consistent across 7+ subcommands (install, switch, delete, diff, apply, enter, rollback). Before applying this refactor broadly, verify that removingSetContext()doesn't affect command tree context propagation or any code relying on that call.cmd/generation/switch/switch.go (3)
45-55: Guard against missing logger in context (panic risk).logger.FromContext() panics if the logger wasn’t injected earlier (e.g., atypical entrypoints/tests). Either ensure the root command always injects it before any subcommand PreRun or add a safe fallback here.
Please confirm the root command guarantees logger injection before PreRun. If not, I can propose a
FromContextOrDefaulthelper.
139-141: LGTM on centralized diff logging.Using system runner’s logger + DiffCommandOptions avoids per-call verbose checks. Good alignment with the new interface.
185-185: LGTM on SetNixProfileGeneration call.Correctly uses genOpts.ProfileName; consistent with DI via system runner.
cmd/generation/shared/utils.go (1)
8-8: Signature change looks good.Value-typed logger aligns with the new interface usage across the repo.
cmd/generation/rollback/rollback.go (3)
30-40: Same PreRun panic risk as switch.Ensure logger is injected before PreRun or add a defensive fallback here.
Confirm top-level wiring guarantees logger in context.
87-89: LGTM on diff invocation.Centralized diff with opts.UseNvd via system runner is consistent.
133-136: LGTM on profile switch.Uses genOpts.ProfileName correctly when setting the profile.
cmd/repl/repl.go (1)
137-137: LGTM on updated FindConfiguration call.Matches the new signature (log, cfg, includes). No further changes needed here.
Ensure the root command injects a logger into context before invoking repl, otherwise logger.FromContext() will panic.
internal/system/local.go (1)
71-73: LGTM on Logger() signature change.Matches the new CommandRunner interface semantics.
cmd/init/filesystems.go (1)
243-257: LGTM: simple bcachefs presence checkThe device prefix heuristic is fine for now.
cmd/generation/list/tui.go (1)
212-241: LGTM: exec wrapper helpers now accept logger by valueCommand echo via CmdArray before exec is consistent with the new logger interface.
cmd/generation/delete/delete.go (1)
84-95: PreRun logger level wiring looks goodLevel is derived from -v and stored back into context; consistent with the new model.
cmd/install/install.go (1)
184-214: Channel copy: echo argv before run (already done) — LGTMUsing s.Logger() inside the helper aligns with the refactor; args assembly looks correct.
internal/logger/logger.go (1)
6-9: LGTM: clear levels and cohesive interfaceLevels and interface surface look consistent with call sites.
Also applies to: 12-26
internal/activation/activation.go (1)
81-98: Review comment is based on incorrect logic analysis and should not be followed as written.The current
if profile != "system"condition in bothAddNewNixProfileandSetNixProfileGenerationis correct. This pattern is consistent throughout the codebase: user profiles (anything other than "system") require theNixSystemProfileDirectoryto exist, while the system profile lives elsewhere inNixProfileDirectory. Inverting this condition would break the logic.However, the exit code handling improvement is valid:
s.Run()returns(int, error)and the current code discards the exit code with_. Adding proper exit code checking (as shown in the diff) is a good defensive improvement since nix-env can fail silently with a non-zero exit code.Recommendation:
- Reject the logic inversion (keep
if profile != "system")- Accept the exit code handling changes (capture and check the exit code)
Likely an incorrect or invalid review comment.
cmd/apply/apply.go (8)
74-84: LGTM! Logger initialization in PreRun is well structured.The PreRun hook correctly retrieves the logger from context, adjusts the log level based on the verbose flag, and propagates it through the command context for use in RunE. This approach cleanly centralizes log-level configuration before command execution.
191-207: LGTM! Configuration lookup correctly refactored.The unconditional
Stepcall is appropriate for this high-level operation, and theFindConfigurationcall correctly matches the simplified signature without the verbose parameter. The logger now handles verbosity internally via log levels.
266-271: LGTM! Appropriate use of debug-level logging.The change from
InfotoDebugffor theNIXOS_GENERATION_TAGdiagnostic is appropriate. This verbose implementation detail should only appear at debug log level.
334-339: LGTM! Debug logging appropriately scoped.The conditional debug message correctly indicates dry build behavior at the appropriate log level.
341-348: LGTM! Diff command call correctly updated.The
RunDiffCommandcall matches the simplified signature, correctly passing theCommandRunnerwhich provides the logger internally. The refactoring removes the explicit logger parameter passing.
390-396: LGTM! Profile activation correctly refactored.The unconditional
Stepcall and theAddNewNixProfileinvocation match the simplified signatures. The logger is correctly provided through theCommandRunnerparameter.
417-422: LGTM! Profile rollback correctly updated.The
SetNixProfileGenerationcall matches the simplified signature, passing theCommandRunnerwhich provides the logger internally.
455-486: LGTM! Channel upgrade refactored consistently.The
upgradeChannelsOptionsstruct correctly removes theVerbosefield, and the function uses thes.Logger()pattern consistently to access the logger from theCommandRunner. The refactoring aligns with the goal of removing explicit verbose parameters.
d20ea5a to
2febbac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/configuration/flake.go (1)
107-116: Guard against nil opts to prevent panic.opts is dereferenced without a nil-check (e.g., UseNom), causing a nil-pointer panic if callers pass nil.
Apply this diff near the start of BuildSystem:
func (f *FlakeRef) BuildSystem(buildType SystemBuildType, opts *SystemBuildOptions) (string, error) { - if f.Builder == nil { + if f.Builder == nil { panic("FlakeRef.Builder is nil") } + if opts == nil { + opts = &SystemBuildOptions{} + }cmd/generation/diff/diff.go (1)
31-41: Fix error messages to echo user input, not the parsed int.On parse failure, the reported value is the zero-value int, not the original arg.
Apply this diff:
- before, err := strconv.ParseInt(args[0], 10, 32) + before, err := strconv.ParseInt(args[0], 10, 32) if err != nil { - return fmt.Errorf("{BEFORE} must be an integer, got '%v'", before) + return fmt.Errorf("{BEFORE} must be an integer, got '%s'", args[0]) } @@ - after, err := strconv.ParseInt(args[1], 10, 32) + after, err := strconv.ParseInt(args[1], 10, 32) if err != nil { - return fmt.Errorf("{AFTER} must be an integer, got '%v'", after) + return fmt.Errorf("{AFTER} must be an integer, got '%s'", args[1]) }cmd/install/install.go (2)
135-178: Path permission check iterates components incorrectly; fix and tighten messagefilepath.SplitList splits PATH-style lists, not path components. This loop only checks the full path, not each component as intended. Also, the hint string is missing a closing backtick.
Proposed fix:
func validateMountpoint(log logger.Logger, mountpoint string) error { @@ - // Check permissions for the mountpoint. All components in the - // mountpoint directory must have an "other users" bit set to at - // least 5 (read+execute). - currentPath := "/" - for _, component := range filepath.SplitList(mountpoint) { + // Check permissions for the mountpoint. All path components must grant o+rx. + clean := filepath.Clean(mountpoint) + if !filepath.IsAbs(clean) { + return fmt.Errorf("mountpoint %s must be an absolute path", mountpoint) + } + currentPath := string(os.PathSeparator) + for _, component := range strings.Split(strings.TrimPrefix(clean, string(os.PathSeparator)), string(os.PathSeparator)) { if component == "" { continue } - currentPath = filepath.Join(currentPath, component) + currentPath = filepath.Join(currentPath, component) info, err := os.Stat(currentPath) if err != nil { return fmt.Errorf("failed to stat %s: %w", currentPath, err) } mode := info.Mode() hasCorrectPermission := mode.Perm()&0o005 >= 0o005 if !hasCorrectPermission { msg := fmt.Sprintf("path %s should have permissions 755, but had permissions %s", currentPath, mode.Perm()) log.Errorf(msg) - log.Printf("hint: consider running `chmod o+rx %s", currentPath) + log.Printf("hint: consider running `chmod o+rx %s`", currentPath) return fmt.Errorf("%v", msg) } }
467-479: Fix user hint text and include actual root path; optional: move step under conditionThe hint shows “{s}” and has a grammar issue. Also consider only printing the “Setting root password…” step when it will actually run.
- log.Step("Setting root password...") - - if !opts.NoRootPassword { - manualHint := "you can set the root password manually by executing `nixos enter --root {s}` and then running `passwd` in the shell of them new system" + if !opts.NoRootPassword { + log.Step("Setting root password...") + manualHint := fmt.Sprintf("you can set the root password manually by executing `nixos enter --root %s` and then running `passwd` in the shell of the new system", mountpoint)
♻️ Duplicate comments (6)
internal/configuration/flake.go (1)
139-145: Reuse the local logger; avoid duplicate calls.You already stored f.Builder.Logger() in log; use it for CmdArray to avoid redundant calls.
Apply this diff:
- log := f.Builder.Logger() + log := f.Builder.Logger() if log.GetLogLevel() == logger.LogLevelDebug { argv = append(argv, "-v") } - f.Builder.Logger().CmdArray(argv) + log.CmdArray(argv)cmd/generation/rollback/rollback.go (2)
27-30: CLI help fixed.Use string now matches behavior (“rollback [flags]”). Good.
157-161: Correct profile used during rollback.Now uses genOpts.ProfileName instead of hard-coded "system". Thanks for fixing.
cmd/generation/switch/switch.go (1)
209-213: Rollback now targets the correct profile — good catchUsing genOpts.ProfileName fixes the previous “system”-hardcode bug. LGTM.
internal/configuration/legacy.go (1)
148-154: Reuse local logger; avoid duplicate method callsYou already assign log := l.Builder.Logger(); reuse it for CmdArray to keep consistency and avoid repeated calls.
- log := l.Builder.Logger() + log := l.Builder.Logger() if log.GetLogLevel() == logger.LogLevelDebug { argv = append(argv, "-v") } - - l.Builder.Logger().CmdArray(argv) + log.CmdArray(argv)internal/configuration/configuration.go (1)
65-66: Correct format string for legacy config path — niceUsing c.ConfigDirname fixes the previous %s pointer formatting issue. Looks good.
🧹 Nitpick comments (12)
internal/logger/syslog.go (4)
97-111: Don’t drop syslog write failures for errors; add stderr fallback.Silent failures on Err/Errf lose critical diagnostics if syslog is unavailable.
Apply this diff to fall back to stderr:
func (l *SyslogLogger) Error(v ...any) { if l.level > LogLevelError { return } - _ = l.writer.Err(fmt.Sprint(v...)) + if err := l.writer.Err(fmt.Sprint(v...)); err != nil { + // best-effort fallback + _, _ = fmt.Fprintln(os.Stderr, fmt.Sprint(v...)) + } } func (l *SyslogLogger) Errorf(format string, v ...any) { if l.level > LogLevelError { return } - _ = l.writer.Err(fmt.Sprintf(format, v...)) + if err := l.writer.Err(fmt.Sprintf(format, v...)); err != nil { + // best-effort fallback + _, _ = fmt.Fprintf(os.Stderr, format+"\n", v...) + } }
1-9: Add a build tag to avoid Windows build failures (log/syslog is not supported).This file won’t compile on Windows. Guard with a build tag.
Apply this diff at the top:
+//go:build !windows package loggerOptionally add a stub for Windows:
+// internal/logger/syslog_stub.go +//go:build windows +package logger + +type SyslogLogger struct{} +func NewSyslogLogger(tag string) (*SyslogLogger, error) { return &SyslogLogger{}, nil } +func (l *SyslogLogger) SetLogLevel(level LogLevel) {} +func (l *SyslogLogger) GetLogLevel() LogLevel { return LogLevelInfo } +func (l *SyslogLogger) Debug(v ...any) {} +func (l *SyslogLogger) Debugf(format string, v ...any) {} +func (l *SyslogLogger) Info(v ...any) {} +func (l *SyslogLogger) Infof(format string, v ...any) {} +func (l *SyslogLogger) Warn(v ...any) {} +func (l *SyslogLogger) Warnf(format string, v ...any) {} +func (l *SyslogLogger) Error(v ...any) {} +func (l *SyslogLogger) Errorf(format string, v ...any) {} +func (l *SyslogLogger) Print(v ...any) {} +func (l *SyslogLogger) Printf(format string, v ...any) {} +func (l *SyslogLogger) CmdArray(argv []string) {} +func (l *SyslogLogger) Step(message string) {}Is Windows a supported target for this repo? If not, we can still add the build tag to be explicit.
11-17: Consider thread-safety if steps may be logged concurrently.stepNumber increments aren’t synchronized; if Step can be called from multiple goroutines, numbers may race.
If concurrency is possible, guard with a mutex or use atomic operations.
25-31: Optional: expose Close() to release the syslog writer.syslog.Writer supports Close(); adding a passthrough avoids keeping the socket open unnecessarily.
Add:
func (l *SyslogLogger) Close() error { return l.writer.Close() }You don’t need to change the Logger interface; this is a type-specific affordance.
internal/configuration/flake.go (1)
107-111: Prefer returning an error over panicking on nil Builder.BuildSystem already returns (string, error); panics hurt CLI UX and complicate recovery.
Return a descriptive error instead:
- if f.Builder == nil { - panic("FlakeRef.Builder is nil") - } + if f.Builder == nil { + return "", fmt.Errorf("BuildSystem: FlakeRef.Builder is nil") + }cmd/generation/rollback/rollback.go (3)
30-40: PreRun re-injects the same logger; reinsertion is optional.You mutate the existing logger’s level; WithLogger(ctx, log) is redundant unless you swap the instance.
You can drop the WithLogger call here to reduce noise.
130-136: Message clarity: include actual profile name, not “system”.Logs say “Setting system profile” / “failed to set system profile” even when profileName ≠ "system".
Apply this diff:
- log.Step("Setting system profile...") + log.Step(fmt.Sprintf("Setting profile '%s'...", genOpts.ProfileName)) @@ - log.Errorf("failed to set system profile: %v", err) + log.Errorf("failed to set profile '%s': %v", genOpts.ProfileName, err)
184-205: Avoid panic when current generation not found.findPreviousGeneration panics on missing current generation; prefer returning an error for better UX.
Replace the panic with an error return to keep commands recoverable in odd states.
cmd/generation/diff/diff.go (1)
46-56: PreRun reinserts the same logger; it’s optional.Same as rollback: WithLogger(ctx, log) isn’t needed unless replacing the instance.
You can remove the reinsertion here.
cmd/generation/switch/switch.go (2)
77-83: Use shared helper to compute profile directoryAvoid duplicating profile path rules and potential inversions. Use generation.GetProfileDirectoryFromName(profileName) as elsewhere.
- profileDirectory := constants.NixProfileDirectory - if profileName != "system" { - profileDirectory = constants.NixSystemProfileDirectory - } + profileDirectory := generation.GetProfileDirectoryFromName(profileName)
117-122: Same: compute profile directory via helperKeep logic consistent with delete command and internal helpers.
- profileDirectory := constants.NixProfileDirectory - if genOpts.ProfileName != "system" { - profileDirectory = constants.NixSystemProfileDirectory - } + profileDirectory := generation.GetProfileDirectoryFromName(genOpts.ProfileName)internal/configuration/legacy.go (1)
116-121: Prefer returning an error over panic for missing BuilderPanicking here will crash the CLI; returning an error keeps failure graceful.
- if l.Builder == nil { - panic("LegacyConfiguration.Builder is nil") - } + if l.Builder == nil { + return "", fmt.Errorf("LegacyConfiguration.Builder is nil") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
cmd/apply/apply.go(9 hunks)cmd/enter/enter.go(2 hunks)cmd/enter/run.go(8 hunks)cmd/generation/delete/delete.go(3 hunks)cmd/generation/diff/diff.go(2 hunks)cmd/generation/rollback/rollback.go(5 hunks)cmd/generation/switch/switch.go(4 hunks)cmd/install/install.go(15 hunks)cmd/option/completion.go(1 hunks)cmd/option/option.go(1 hunks)cmd/repl/repl.go(1 hunks)internal/activation/activation.go(5 hunks)internal/configuration/configuration.go(2 hunks)internal/configuration/flake.go(4 hunks)internal/configuration/legacy.go(4 hunks)internal/generation/diff.go(2 hunks)internal/generation/specialisations.go(1 hunks)internal/logger/console.go(1 hunks)internal/logger/logger.go(1 hunks)internal/logger/multi.go(1 hunks)internal/logger/syslog.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/generation/specialisations.go
- internal/activation/activation.go
- internal/logger/multi.go
- cmd/enter/enter.go
- cmd/option/completion.go
- cmd/option/option.go
- internal/generation/diff.go
- internal/logger/console.go
🧰 Additional context used
🧬 Code graph analysis (12)
internal/logger/syslog.go (2)
internal/logger/logger.go (5)
LogLevel(3-3)LogLevelInfo(7-7)LogLevelDebug(6-6)LogLevelWarn(8-8)LogLevelError(9-9)internal/utils/utils.go (1)
EscapeAndJoinArgs(27-41)
internal/configuration/flake.go (1)
internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)
cmd/apply/apply.go (6)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/configuration/configuration.go (1)
FindConfiguration(43-69)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/constants/constants.go (1)
CurrentSystem(7-7)internal/activation/activation.go (2)
AddNewNixProfile(62-79)SetNixProfileGeneration(81-98)
cmd/generation/rollback/rollback.go (4)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/activation/activation.go (1)
SetNixProfileGeneration(81-98)
cmd/install/install.go (3)
internal/system/runner.go (2)
Command(15-22)CommandRunner(10-13)internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)
cmd/enter/run.go (2)
internal/logger/logger.go (1)
Logger(12-27)internal/system/runner.go (1)
CommandRunner(10-13)
cmd/generation/switch/switch.go (4)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (1)
LogLevelDebug(6-6)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/activation/activation.go (1)
SetNixProfileGeneration(81-98)
cmd/generation/diff/diff.go (3)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (1)
LogLevelDebug(6-6)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)
cmd/generation/delete/delete.go (6)
internal/system/runner.go (3)
Command(15-22)CommandRunner(10-13)NewCommand(24-33)internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/constants/constants.go (1)
CurrentSystem(7-7)internal/build/build.go (1)
Flake(32-34)internal/build/vars/vars.go (1)
Flake(9-9)
internal/configuration/configuration.go (5)
internal/logger/logger.go (1)
Logger(12-27)internal/settings/settings.go (1)
Settings(15-27)internal/build/build.go (1)
Flake(32-34)internal/build/vars/vars.go (1)
Flake(9-9)internal/configuration/legacy.go (1)
FindLegacyConfiguration(24-82)
cmd/repl/repl.go (1)
internal/configuration/configuration.go (1)
FindConfiguration(43-69)
internal/configuration/legacy.go (1)
internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (22)
cmd/repl/repl.go (1)
137-141: Signature migration looks correct.FindConfiguration now derives verbosity from the logger; callsite adapts properly and preserves error handling.
cmd/generation/rollback/rollback.go (1)
87-89: Diff callsite update looks good.RunDiffCommand now derives logging from the system. Correctly passes UseNvd from config.
cmd/generation/diff/diff.go (1)
87-89: Callsite update LGTM.RunDiffCommand now pulls its logger from the system; options align with new struct.
cmd/generation/delete/delete.go (3)
84-94: Ensure logger is injected before PreRunSame consideration: logger.FromContext will panic if not set. Confirm root command injects the logger for all command trees.
You can reuse the script from the switch comment to verify.
232-243: Deletion command logging looks consistentCmdArray before execution and simplified signature (no verbose param) align with the new logger model. LGTM.
257-281: Verbosity-driven GC is implemented correctlySwitching between “nix store gc” and “nix-collect-garbage” and adding -v only at debug level is sensible. LGTM.
internal/configuration/legacy.go (1)
24-31: Debug logs for discovery are appropriateMoving to Debugf and dropping explicit verbose plumbing is consistent with the new logger. LGTM.
cmd/install/install.go (4)
62-72: Verify logger injection before PreRunAs elsewhere, logger.FromContext panics if missing. Confirm root initializes the logger for this command too.
184-214: Channel copy logging and runner usage look goodDeriving the logger from s and emitting CmdArray before Run aligns with the new design. LGTM.
244-269: Initial generation creation is consistentGood use of s.Logger().CmdArray and clear error reporting.
280-314: Bootloader install: level-driven -v/-s selection is solidThe verbosity switch based on logger level is clear; command logging is correct.
cmd/generation/switch/switch.go (1)
45-55: Review comment is incorrect; no action neededThe root command already properly initializes and injects the logger before any subcommand PreRun executes. At
cmd/root/root.go:125, the root command callscmd.SetContext(cmdCtx)wherecmdCtxcontains the logger set at line 60. Cobra propagates this context to child commands, so whenswitch.go's PreRun callslogger.FromContext(ctx)at line 47, the logger is already present. No panic will occur.internal/logger/logger.go (1)
5-27: LGTM! Clean interface design.The refactor from concrete Logger to interface is well-executed:
- LogLevel constants are clearly defined with Debug as the most verbose level
- Interface methods follow consistent naming patterns (Debug/Debugf, Info/Infof, etc.)
- Addition of GetLogLevel() enables introspection of current log level
cmd/enter/run.go (3)
157-171: LGTM! Consistent logging refactor.The function signature change from accepting a verbose boolean to accepting a logger parameter is correct. The unconditional use of
log.Debugfandlog.CmdArrayproperly delegates verbosity control to the logger's log level.
219-256: LGTM! Appropriate separation of concerns.The function correctly:
- Removes the verbose parameter in favor of logger-based control
- Retains the
silentparameter for controlling stdout/stderr redirection (distinct from log verbosity)- Uses unconditional
s.Logger().CmdArray(argv)calls, delegating verbosity to log level
258-271: LGTM!Consistent with the broader refactor—unconditional command logging via
s.Logger().CmdArray(argv).cmd/apply/apply.go (6)
191-191: LGTM!Unconditional logging is appropriate here—the logger's configured level will control visibility.
201-201: LGTM!Consistent with the broader refactor to remove verbose parameter passing. The logger is now passed via context and
FindConfigurationcan retrieve it internally if needed.
268-268: LGTM! Appropriate use of debug logging.Using
log.Debugffor verbose-only information aligns with the new log-level-based approach.Also applies to: 335-337
343-345: LGTM!The signature change to remove the logger parameter is correct—
RunDiffCommandaccepts asystem.CommandRunnerwhich provides access to the logger via theLogger()method (as shown in the relevant code snippets).
391-393: LGTM! Consistent refactor.All function calls correctly:
- Remove verbose parameters
- Use unconditional
CmdArraylogging- Rely on the logger's configured level for verbosity control
Also applies to: 418-418, 481-481
74-84: Ignore the original review comment.The original concern is unfounded. Here's why:
All logger implementations use pointer receivers for SetLogLevel (confirmed via AST-grep):
*ConsoleLogger,*SyslogLogger,*MultiLoggerall have pointer receiver methods.The Logger interface preserves pointer semantics: When
FromContext()retrieves the logger, it returns aLoggerinterface value that wraps a pointer to the concrete logger (e.g.,*ConsoleLogger). In Go, interface values that wrap pointer types preserve pointer semantics across storage and retrieval.SetLogLevel mutations persist: When PreRun calls
log.SetLogLevel(logger.LogLevelDebug), this invokes the pointer receiver method on the underlying concrete logger, mutating the object. SinceWithLogger()stores andFromContext()retrieves the same interface (wrapping the same pointer), the mutation is visible whenapplyMainlater callslogger.FromContext().Context.WithValue handles pointers correctly: Storing an interface value that wraps a pointer in context preserves that pointer across storage/retrieval cycles.
The code is correct—no fix is needed.
2febbac to
6709ae9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
internal/logger/syslog.go (1)
113-119: Issue already resolved.The past review comment flagged that CmdArray was calling
l.writer.Infoinstead ofl.writer.Debug. The current code correctly callsl.writer.Debugat line 118, so the severity mismatch has been fixed.internal/logger/console.go (1)
11-16: Issue already resolved.The past review comment flagged that the blue color variable was using
color.FgRed. The current code correctly usescolor.FgBlueat line 15.cmd/generation/rollback/rollback.go (2)
27-27: Issue already resolved.The past review comment flagged that the Use string incorrectly advertised a
{GEN}argument. The current code correctly shows"rollback [flags]"without the positional argument.
157-158: Issue already resolved.The past review comment flagged that this call hard-coded "system" instead of using the profile name variable. The current code correctly uses
genOpts.ProfileNameat line 158.internal/configuration/legacy.go (1)
117-170: Nil check is now properly placed; reuse the local logger.The nil check for
l.Builderhas been correctly moved to the beginning of the function (lines 117-119), addressing the nil-deref risk from the previous review. However, the logger is still retrieved twice: once at line 148 (log := l.Builder.Logger()) and again at line 153 (l.Builder.Logger().CmdArray(argv)). Reuse the locallogvariable for consistency.Apply this diff:
log := l.Builder.Logger() if log.GetLogLevel() == logger.LogLevelDebug { argv = append(argv, "-v") } - l.Builder.Logger().CmdArray(argv) + log.CmdArray(argv)internal/configuration/configuration.go (1)
43-69: Clean refactor with past format issue resolved.The signature change removing the
verboseparameter and the conversion to debug-level logging is consistent with the PR's objectives. The previous format verb issue (printing pointer with%s) is now correctly resolved at line 65 by usingc.ConfigDirname.
🧹 Nitpick comments (4)
cmd/generation/delete/delete.go (2)
84-94: PreRun hook correctly sets up logger based on verbose flag.The pattern of retrieving the logger, conditionally modifying it, and storing it back in context is appropriate for this refactoring.
Minor optional optimization: the logger is stored back in context even when
opts.Verboseis false and no modification occurred. Consider wrapping lines 92-93 in the sameif opts.Verboseblock to avoid the unnecessary context update.if opts.Verbose { log.SetLogLevel(logger.LogLevelDebug) + ctx = logger.WithLogger(ctx, log) + cmd.SetContext(ctx) } - - ctx = logger.WithLogger(ctx, log) - cmd.SetContext(ctx)
257-281: collectGarbage correctly derives verbosity from log level, but inefficiently retrieves logger twice.The logic to add the
-vflag based onLogLevelDebugis correct and aligns with the refactoring goals. However, the logger is retrieved twice: once at line 265 (log := s.Logger()) and again at line 270 (s.Logger().CmdArray(argv)).Reuse the
logvariable from line 265:log := s.Logger() if log.GetLogLevel() == logger.LogLevelDebug { argv = append(argv, "-v") } - s.Logger().CmdArray(argv) + log.CmdArray(argv)internal/generation/diff.go (1)
13-37: Reuse the local logger variable for consistency.The logger is retrieved twice: once at line 14 (
log := s.Logger()) and again at line 33 (s.Logger().CmdArray(argv)). Reuse the locallogvariable for consistency and to avoid redundant method calls.Apply this diff:
func RunDiffCommand(s system.CommandRunner, before string, after string, opts *DiffCommandOptions) error { log := s.Logger() useNvd := opts.UseNvd if opts.UseNvd { nvdPath, _ := exec.LookPath("nvd") nvdFound := nvdPath != "" if !nvdFound { log.Warn("use_nvd is specified in config, but `nvd` is not executable") log.Warn("falling back to `nix store diff-closures`") useNvd = false } } argv := []string{"nix", "store", "diff-closures", before, after} if useNvd { argv = []string{"nvd", "diff", before, after} } - s.Logger().CmdArray(argv) + log.CmdArray(argv) cmd := system.NewCommand(argv[0], argv[1:]...) _, err := s.Run(cmd) return err }cmd/install/install.go (1)
316-333: Reuse the local logger variable.The logger is retrieved at line 317 (
log := s.Logger()) but thens.Logger()is called again at line 326. Reuse the locallogvariable for consistency.Apply this diff:
func setRootPassword(s system.CommandRunner, mountpoint string) error { log := s.Logger() argv := []string{os.Args[0], "enter", "--root", mountpoint, "-c", "/nix/var/nix/profiles/system/sw/bin/passwd"} if log.GetLogLevel() == logger.LogLevelDebug { argv = append(argv, "-v") } else { argv = append(argv, "-s") } - s.Logger().CmdArray(argv) + log.CmdArray(argv) cmd := system.NewCommand(argv[0], argv[1:]...) cmd.SetEnv("NIXOS_CLI_DISABLE_STEPS", "1") _, err := s.Run(cmd) return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
cmd/activate/run.go(2 hunks)cmd/apply/apply.go(9 hunks)cmd/enter/enter.go(2 hunks)cmd/enter/run.go(8 hunks)cmd/generation/delete/delete.go(3 hunks)cmd/generation/diff/diff.go(2 hunks)cmd/generation/rollback/rollback.go(5 hunks)cmd/generation/switch/switch.go(4 hunks)cmd/install/install.go(15 hunks)cmd/option/completion.go(1 hunks)cmd/option/option.go(1 hunks)cmd/repl/repl.go(1 hunks)internal/activation/activation.go(5 hunks)internal/configuration/configuration.go(2 hunks)internal/configuration/flake.go(4 hunks)internal/configuration/legacy.go(4 hunks)internal/generation/diff.go(2 hunks)internal/generation/specialisations.go(1 hunks)internal/logger/console.go(1 hunks)internal/logger/logger.go(1 hunks)internal/logger/multi.go(1 hunks)internal/logger/syslog.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/generation/switch/switch.go
- internal/configuration/flake.go
- cmd/option/option.go
- internal/logger/multi.go
- cmd/repl/repl.go
- internal/activation/activation.go
- cmd/enter/run.go
🧰 Additional context used
🧬 Code graph analysis (14)
cmd/generation/diff/diff.go (3)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (1)
LogLevelDebug(6-6)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)
cmd/generation/delete/delete.go (6)
internal/system/runner.go (3)
Command(15-22)CommandRunner(10-13)NewCommand(24-33)internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/constants/constants.go (1)
CurrentSystem(7-7)internal/build/vars/vars.go (1)
Flake(9-9)internal/build/build.go (1)
Flake(32-34)
internal/logger/syslog.go (2)
internal/logger/logger.go (5)
LogLevel(3-3)LogLevelInfo(7-7)LogLevelDebug(6-6)LogLevelWarn(8-8)LogLevelError(9-9)internal/utils/utils.go (1)
EscapeAndJoinArgs(27-41)
cmd/enter/enter.go (2)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (1)
LogLevelDebug(6-6)
cmd/generation/rollback/rollback.go (6)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/constants/constants.go (1)
CurrentSystem(7-7)internal/activation/activation.go (1)
SetNixProfileGeneration(81-98)internal/generation/generation.go (1)
Generation(27-38)
cmd/option/completion.go (4)
internal/logger/logger.go (1)
Logger(12-27)internal/settings/settings.go (1)
Settings(15-27)internal/system/local.go (1)
NewLocalSystem(17-21)internal/configuration/configuration.go (1)
FindConfiguration(43-69)
internal/logger/console.go (2)
internal/logger/logger.go (6)
Logger(12-27)LogLevel(3-3)LogLevelInfo(7-7)LogLevelDebug(6-6)LogLevelWarn(8-8)LogLevelError(9-9)internal/utils/utils.go (1)
EscapeAndJoinArgs(27-41)
cmd/activate/run.go (3)
internal/logger/logger.go (1)
Logger(12-27)internal/logger/syslog.go (1)
NewSyslogLogger(19-31)internal/logger/multi.go (1)
NewMultiLogger(8-10)
internal/configuration/legacy.go (1)
internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)
internal/generation/specialisations.go (1)
internal/configuration/configuration.go (1)
FindConfiguration(43-69)
internal/configuration/configuration.go (5)
internal/logger/logger.go (1)
Logger(12-27)internal/settings/settings.go (1)
Settings(15-27)internal/build/vars/vars.go (1)
Flake(9-9)internal/build/build.go (1)
Flake(32-34)internal/configuration/legacy.go (1)
FindLegacyConfiguration(24-82)
internal/generation/diff.go (2)
internal/system/runner.go (1)
CommandRunner(10-13)internal/logger/logger.go (1)
Logger(12-27)
cmd/apply/apply.go (5)
internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)internal/configuration/configuration.go (1)
FindConfiguration(43-69)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/activation/activation.go (2)
AddNewNixProfile(62-79)SetNixProfileGeneration(81-98)
cmd/install/install.go (3)
internal/system/runner.go (2)
Command(15-22)CommandRunner(10-13)internal/logger/context.go (2)
FromContext(13-20)WithLogger(9-11)internal/logger/logger.go (2)
LogLevelDebug(6-6)Logger(12-27)
🔇 Additional comments (25)
cmd/generation/delete/delete.go (3)
183-200: Function calls correctly updated to new signatures.The removal of
verboseparameters fromdeleteGenerations,regenerateBootMenu, andcollectGarbagecalls is consistent with the refactored function signatures and aligns with the PR objectives to eliminate scattered verbose flag checks.
232-243: deleteGenerations refactored correctly.The function signature change removing the
verboseparameter and the unconditionalCmdArraycall align with the log-level-driven approach. The Logger implementation should handle filtering command output based on the current log level.
245-255: regenerateBootMenu refactored correctly.Consistent with the other refactored functions, this correctly removes the
verboseparameter and delegates verbosity control to the Logger implementation.cmd/enter/enter.go (2)
18-24: LGTM!The Args handler correctly populates the command array from positional arguments.
25-35: No action required—logger is properly initialized before PreRun execution.The root command (cmd/root/root.go:58-59) initializes the logger and attaches it to the context via
logger.WithLogger(), then explicitly sets it on the root command withSetContext(cmdCtx)at line 125. Since the enter command is registered as a subcommand and Cobra propagates the parent's context to child commands, the context passed to enter's PreRun already contains the logger. The call tologger.FromContext(ctx)will succeed safely.Likely an incorrect or invalid review comment.
cmd/activate/run.go (2)
105-110: LGTM!The signature change from
*logger.Loggertologger.Loggeraligns with the PR's objective to use value-based loggers throughout the codebase.
385-389: LGTM!The syslog logger initialization is well-handled:
- Gracefully degrades if syslog is unavailable
- Uses MultiLogger to combine console and syslog output
- Appropriately logs a warning if initialization fails
internal/logger/syslog.go (2)
121-134: LGTM!The Step method correctly handles both enabled and disabled states:
- When disabled, delegates to Info with proper level checking
- When enabled, applies level gating before emitting step messages
19-31: LGTM!The constructor appropriately initializes the SyslogLogger:
- Uses LOG_DEBUG priority to allow application-level filtering
- Sets sensible defaults (Info level, steps enabled)
- Consistent with ConsoleLogger initialization
internal/logger/console.go (3)
30-42: LGTM!The constructor properly initializes the ConsoleLogger with:
- Appropriate colored prefixes for each log level
- Output directed to stderr (standard practice for logging)
- Sensible defaults matching other logger implementations
122-129: LGTM!The CmdArray method correctly:
- Gates output with debug level check
- Formats command with visual distinction
- Outputs through the debug logger
131-148: LGTM!The Step method implementation is well-designed:
- Properly handles enabled/disabled states
- Adds visual spacing between steps for readability
- Uses distinctive formatting (magenta bold numbering)
- Outputs via print logger to avoid redundant "info:" prefix
internal/generation/specialisations.go (1)
99-99: LGTM!The FindConfiguration call correctly uses the updated three-argument signature, removing the verbose flag in favor of logger-driven verbosity control.
cmd/generation/diff/diff.go (2)
87-89: LGTM!The RunDiffCommand call correctly uses the updated signature, removing explicit logger/verbose parameters in favor of obtaining the logger from the CommandRunner.
46-56: Logger is properly initialized and propagated to child commands; no action needed.The verification shows that logger initialization is correctly handled:
- The root command creates a logger via
logger.NewConsoleLogger()and attaches it to the context withlogger.WithLogger(context.Background(), log)- The root command then calls
cmd.SetContext(cmdCtx)to persist this context on the command- According to Cobra's documented behavior, when a context is set with
SetContext, the previously set context is returned to child commands- Child commands like
diff.goreceive this context viacmd.Context()and can safely retrieve the loggerThe concern about
logger.FromContext()panicking is not applicable here—the logger is guaranteed to be present in the context when child command PreRun hooks execute.cmd/option/completion.go (2)
16-16: LGTM!The signature change from
*logger.Loggertologger.Loggeraligns with the PR's objective to use value-based loggers throughout the codebase.
19-19: LGTM!The FindConfiguration call correctly uses the updated three-argument signature, removing the verbose flag in favor of logger-driven verbosity control.
cmd/generation/rollback/rollback.go (4)
87-89: LGTM!The RunDiffCommand call correctly uses the updated signature, removing explicit logger/verbose parameters in favor of obtaining the logger from the CommandRunner.
133-133: LGTM!The call correctly uses
genOpts.ProfileNameinstead of a hard-coded "system" string, ensuring the correct profile is set for rollback.
184-184: LGTM!The signature change from
*logger.Loggertologger.Loggeraligns with the PR's objective to use value-based loggers throughout the codebase.
30-40: No action required—logger is properly initialized in the context.The root command initializes a logger via
logger.NewConsoleLogger()and embeds it in the context withlogger.WithLogger(context.Background(), log)at lines 58–59 ofcmd/root/root.go. The context is then set on the root command viacmd.SetContext(cmdCtx)at line 125 before child commands are added. Cobra automatically inherits the parent context to child commands, so the rollback PreRun receives the context with the logger already embedded. The generation command already uses the same pattern in itsPersistentPreRunEhook, which callslogger.FromContext(cmd.Context()), confirming that context propagation works correctly.Likely an incorrect or invalid review comment.
internal/logger/logger.go (1)
5-27: Clean interface refactor with proper log-level hierarchy.The conversion from a concrete Logger to an interface with the addition of
Debug/Debugfmethods andGetLogLevel()is well-structured. The log-level ordering (Debug=0, Info=1, Warn=2, Error=3) enables natural threshold comparisons for verbosity filtering.cmd/install/install.go (1)
62-72: Well-structured PreRun hook for verbose propagation.The PreRun hook cleanly propagates the
--verboseflag to the logger's debug level and ensures the updated logger is stored back in the context. This approach eliminates the need to thread verbose flags through function parameters.cmd/apply/apply.go (2)
74-84: PreRun hook properly initializes context-based logging.The PreRun hook cleanly sets the debug level based on the
--verboseflag and propagates the logger through the context, eliminating the need for explicit verbose flag threading throughout the call chain.
163-450: Call sites correctly updated for new logger-driven signatures.All function calls have been properly updated to match the refactored signatures that remove explicit logger and verbose parameters (e.g.,
FindConfiguration,RunDiffCommand,AddNewNixProfile,SetNixProfileGeneration). The logging behavior now depends on the logger's configured level rather than explicit verbose flags.
Description
This PR improves logging facilities within
nixos-cliin the following ways:Loggerfacility into an interface instead of a concrete console logger (this is implemented separately)SyslogLoggerimplementation fornixos activateCloses #110.
Summary by CodeRabbit
New Features
Improvements