feat: add log.file, log.format, log.level config file options#30
feat: add log.file, log.format, log.level config file options#30robbiet480 merged 5 commits intomainfrom
Conversation
CLI flags still take precedence. log.level accepts "debug", "info", or "warn". Useful for always writing debug logs to a file without passing flags on every invocation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDerive effective logging settings from loaded config when CLI flags (debug, verbose, log-file, log-format) are unset; apply those effective values to choose log level, formatter, and output file without mutating flag-backed globals; defer and emit a warning for unknown config log.level after logger is configured. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (CLI)"
participant Root as "Root Command"
participant Config as "Config Loader"
participant Logger as "Logger Setup"
User->>Root: invoke CLI (with or without flags)
Root->>Config: load configuration
Config-->>Root: return Config (contains Log)
Root->>Root: determine effectiveDebug/effectiveVerbose/effectiveLogFile/effectiveLogFormat
Root->>Logger: select formatter (by effectiveLogFormat)
Root->>Logger: open log file (effectiveLogFile) if set
Root->>Logger: set log level (using effective debug/verbose or validated Config.Log.Level)
Logger-->>Root: logger initialized
Root-->>User: start application (emit deferred unknown-level warning if present)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/root.go (1)
59-73: Config-seeded logging logic looks correct; minor consistency suggestion.The implementation correctly prioritizes CLI flags over config values. A few observations:
Boolean flag check (Line 60): Using
!debug && !verboseworks because these flags default tofalse, but it's inconsistent with thecmd.Flags().Changed()approach used forlog-fileandlog-format. For clarity and consistency, consider:Invalid config values: If
Cfg.Log.Levelcontains an unrecognized value (e.g.,"trace"), it silently falls through to the default (warn). This is probably fine, but you could log a warning for discoverability.♻️ Optional: Use Changed() for consistency
// Seed log settings from config file; CLI flags take precedence. - if Cfg.Log.Level != "" && !debug && !verbose { + if Cfg.Log.Level != "" && !cmd.Flags().Changed("debug") && !cmd.Flags().Changed("verbose") { switch strings.ToLower(Cfg.Log.Level) { case "debug": debug = true case "info": verbose = true + case "warn": + // default, no action needed + default: + log.Warnf("Unknown log.level %q in config, using default (warn)", Cfg.Log.Level) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 59 - 73, Replace the implicit check "!debug && !verbose" when seeding Cfg.Log.Level with explicit CLI-flag checks using cmd.Flags().Changed("debug") and cmd.Flags().Changed("verbose") so config only wins when those flags were not provided (i.e., if !cmd.Flags().Changed("debug") && !cmd.Flags().Changed("verbose") then apply Cfg.Log.Level to set debug/verbose); also add a default case for the switch on Cfg.Log.Level that emits a warning (e.g., logger.Warnf or similar) when an unrecognized level like "trace" is encountered to improve discoverability while still falling back to the default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/root.go`:
- Around line 59-73: Replace the implicit check "!debug && !verbose" when
seeding Cfg.Log.Level with explicit CLI-flag checks using
cmd.Flags().Changed("debug") and cmd.Flags().Changed("verbose") so config only
wins when those flags were not provided (i.e., if !cmd.Flags().Changed("debug")
&& !cmd.Flags().Changed("verbose") then apply Cfg.Log.Level to set
debug/verbose); also add a default case for the switch on Cfg.Log.Level that
emits a warning (e.g., logger.Warnf or similar) when an unrecognized level like
"trace" is encountered to improve discoverability while still falling back to
the default behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df110b21-223d-4dac-badb-1ea122012a2c
📒 Files selected for processing (2)
cmd/root.goconfig/config.go
…own level Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/root.go (1)
59-77: Use local effective logging values instead of mutating the flag-backed globals.This block turns
debug,verbose,logFile, andlogFormatinto merged runtime state. That makes later diagnostics ambiguous — e.g. Line 99 reportsinvalid --log-formateven when the bad value came fromsettings.yaml— and makes repeatedLoadConfig()calls depend on those globals being reset elsewhere. Prefer localeffectiveLogLevel/effectiveLogFile/effectiveLogFormatvalues derived from flags + config, then apply them below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 59 - 77, Do not mutate the flag-backed globals (debug, verbose, logFile, logFormat) when seeding from Cfg.Log; instead compute local effective values (e.g. effectiveDebug/effectiveVerbose/effectiveLogFile/effectiveLogFormat) by checking Cfg.Log.* together with cmd.Flags().Changed("debug"|"verbose"|"log-file"|"log-format"), use strings.ToLower(Cfg.Log.Level) to set effectiveDebug/effectiveVerbose (handle "debug"/"info"/"warn"/"warning" and log.Warnf for unknown levels), and then apply those effective variables where logging is initialized later so diagnostics (e.g. the invalid --log-format check) use the effective* values without altering the original flag-backed globals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/root.go`:
- Around line 59-77: Do not mutate the flag-backed globals (debug, verbose,
logFile, logFormat) when seeding from Cfg.Log; instead compute local effective
values (e.g.
effectiveDebug/effectiveVerbose/effectiveLogFile/effectiveLogFormat) by checking
Cfg.Log.* together with
cmd.Flags().Changed("debug"|"verbose"|"log-file"|"log-format"), use
strings.ToLower(Cfg.Log.Level) to set effectiveDebug/effectiveVerbose (handle
"debug"/"info"/"warn"/"warning" and log.Warnf for unknown levels), and then
apply those effective variables where logging is initialized later so
diagnostics (e.g. the invalid --log-format check) use the effective* values
without altering the original flag-backed globals.
Seed log settings from config into local effectiveDebug/effectiveVerbose/ effectiveLogFile/effectiveLogFormat variables rather than mutating the cobra flag-backed globals, so CLI flag state remains unaltered. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 66-76: The current switch on Cfg.Log.Level calls log.Warnf
immediately for unknown values; instead capture that warning text into a local
variable (e.g., unknownLogLevelMsg) inside the existing conditional/switch that
sets effectiveDebug/effectiveVerbose and do not call log.Warnf there, then after
calling setAllLogFormatters(...) and setAllLogOutputs(...) check if
unknownLogLevelMsg is non-empty and emit the warning with
log.Warnf(unknownLogLevelMsg) so the message uses the configured format/output;
reference the existing symbols Cfg.Log.Level, effectiveDebug, effectiveVerbose,
the switch that currently calls log.Warnf, and the functions setAllLogFormatters
and setAllLogOutputs.
- Around line 99-105: The default error message in the switch over
effectiveLogFormat incorrectly references the CLI flag name; update the error
returned in the default branch (the switch handling effectiveLogFormat) to be
source-agnostic — e.g., say "invalid log format %q: must be 'text' or 'json'" —
so it doesn't assume the value came from --log-format; change the string
produced where the switch currently returns fmt.Errorf("invalid --log-format %q:
must be 'text' or 'json'", effectiveLogFormat) to a neutral message using
effectiveLogFormat.
…figured
Also make the invalid log format error message source-agnostic
("invalid log format" rather than "invalid --log-format") since
the value may come from config rather than a CLI flag.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Adds a
logsection tosettings.yamlso logging can be configured without passing flags on every invocation. CLI flags still take precedence.Test plan
go test ./...passeslog.level: debugin config, runaxm2snipe syncwithout-d— debug output appears-von CLI — overrideslog.level: debugdown to infolog.file: axm2snipe.log— log file written on each run🤖 Generated with Claude Code
Summary by CodeRabbit