Style Atmos Logger with Theme #1121
Conversation
- Added unit tests for charmbracelet logger functionality - Renamed `NewLogger` to `InitializeLogger` across tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1121 +/- ##
==========================================
+ Coverage 17.44% 17.87% +0.42%
==========================================
Files 170 172 +2
Lines 18764 18933 +169
==========================================
+ Hits 3274 3384 +110
- Misses 14892 14948 +56
- Partials 598 601 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis pull request updates the logger initialization across several components. The CLI argument parsing in both the Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Parser as CLI Parser Function
participant LoggerInit as InitializeLogger/InitializeLoggerFromCliConfig
participant Logger as Logger Package
participant ErrorHandler as Error Handling
CLI->>Parser: Execute command (e.g., "describe affected")
Parser->>LoggerInit: Call logger initialization with &config
LoggerInit->>Logger: Initialize Logger (with Charmbracelet support)
Logger-->>LoggerInit: Return logger instance or error
alt Logger initialization fails
LoggerInit->>ErrorHandler: Return error and empty args
else Logger initialization succeeds
LoggerInit->>Parser: Return logger instance
Parser->>Parser: Continue processing CLI arguments
end
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 golangci-lint (1.62.2)pkg/logger/test_logger.go19-19: func (unused) 30-30: func (unused) 39-39: func (unused) 54-54: func (unused) 🪛 GitHub Check: golangcipkg/logger/test_logger.go[failure] 19-19: [golangci] pkg/logger/test_logger.go#L19 [failure] 30-30: [golangci] pkg/logger/test_logger.go#L30 [failure] 39-39: [golangci] pkg/logger/test_logger.go#L39 [failure] 54-54: [golangci] pkg/logger/test_logger.go#L54 ⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (8)
cmd/root.go (1)
127-154: Comprehensive output configuration with error handlingThe log output configuration handles all standard cases as well as custom files. Good error handling with fallback to stderr when file operations fail.
Replace the magic number
0o644with a named constant to improve code readability:+// DefaultLogFilePermissions defines standard permissions for log files +const DefaultLogFilePermissions = 0o644 func configureLogOutput(logFile string) { // ... - customFile, err := os.OpenFile(logFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o644) + customFile, err := os.OpenFile(logFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, DefaultLogFilePermissions) // ... }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 133-135: cmd/root.go#L133-L135
Added lines #L133 - L135 were not covered by tests
[warning] 137-138: cmd/root.go#L137-L138
Added lines #L137 - L138 were not covered by tests
[warning] 142-147: cmd/root.go#L142-L147
Added lines #L142 - L147 were not covered by tests
[warning] 153-153: cmd/root.go#L153
Added line #L153 was not covered by tests🪛 GitHub Check: golangci
[failure] 142-142: [golangci] cmd/root.go#L142
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)pkg/logger/charmbracelet.go (5)
109-112: Add test coverage for the globalErrormethod.
Coverage tools indicate these lines are not covered. Consider callinglogger.Error(...)in a dedicated test to ensure coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-111: pkg/logger/charmbracelet.go#L110-L111
Added lines #L110 - L111 were not covered by tests
114-117: Add test coverage for the globalWarnmethod.
Similarly, ensure thatWarnis invoked in tests to improve coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 115-116: pkg/logger/charmbracelet.go#L115-L116
Added lines #L115 - L116 were not covered by tests
119-122: Consider verifyingInfoin tests.
Including a direct test call tologger.Info(...)will strengthen coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-121: pkg/logger/charmbracelet.go#L120-L121
Added lines #L120 - L121 were not covered by tests
124-127: Extend coverage forDebug.
Like the others, confirming a debug log call in tests would be beneficial.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-126: pkg/logger/charmbracelet.go#L125-L126
Added lines #L125 - L126 were not covered by tests
129-132: Test calls toFatalfor completeness.
Even though it terminates the program, consider a controlled test approach or mocking to verify.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-131: pkg/logger/charmbracelet.go#L130-L131
Added lines #L130 - L131 were not covered by testsexamples/quick-start-simple/test_logger.go (1)
21-30: Runnable example is practical but not a standard test.
Since this is in themainfunction, test coverage tools likely won't track it. If coverage is needed, consider a_test.goapproach.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-29: examples/quick-start-simple/test_logger.go#L21-L29
Added lines #L21 - L29 were not covered by testspkg/logger/logger.go (1)
142-145: Improve coverage by callingError(err).
Currently, these lines may remain untested. Testing ensures robust error logging.
🛑 Comments failed to post (1)
pkg/logger/logger.go (1)
161-168: 🛠️ Refactor suggestion
Avoid magic number for file permissions.
Use a named constant for0o644to improve readability and maintainability.- f, err := os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o644) + const filePerm = 0o644 + f, err := os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, filePerm)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// openLogFile opens a log file for writing with appropriate flags. func openLogFile(file string) (*os.File, error) { const filePerm = 0o644 f, err := os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, filePerm) if err != nil { return nil, fmt.Errorf("failed to open log file: %w", err) } return f, nil }🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 163-163: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: codecov/patch
[warning] 165-166: pkg/logger/logger.go#L165-L166
Added lines #L165 - L166 were not covered by tests🪛 GitHub Check: golangci
[failure] 163-163: [golangci] pkg/logger/logger.go#L163
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/root.go (3)
91-106: Well-structured logger initialization.The
setupLoggerfunction cleanly separates logger initialization logic from the main execution flow, with good error handling that defaults to stderr and info level when initialization fails.However, consider if the
atmosLoggervariable has other properties that might be useful beyond just extracting the LogLevel.
108-124: Good log level mapping implementation.The function handles all log levels appropriately with a sensible default fallback to InfoLevel.
Note that for LogLevelOff, using
log.FatalLevel + 1works but depends on internal implementation details of the Charmbracelet library. Consider if there's a more explicit way to disable logging that would be more resilient to library changes.
126-154: Comprehensive log output configuration.The function handles all standard output destinations and custom log files with proper error handling and permissions.
The comments about file descriptor management are helpful. For production environments with long-running applications, consider adding log rotation capabilities to prevent log files from growing indefinitely.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/root.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
cmd/root.go (3)
18-19: Clean import declarations for logger module.The imports are properly organized and include the necessary packages for the refactored logger functionality.
61-66: Improved error handling.The code now uses
errors.Is(err, config.NotFound)instead of type assertion, which is more reliable. The warning message also uses structured logging for better readability.
199-201: Consistent configuration initialization.The error handling approach is consistent with other changes in the file, using
errors.Isfor more reliable error type checking.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/root.go (2)
63-69: Consider simplifying the error handling logic.The current error handling structure can be simplified for better readability.
- if errors.Is(err, config.NotFound) { - // For help commands or when help flag is set, we don't want to show the error - if !isHelpRequested { - log.Warn("CLI configuration issue", "error", err) - } - } else { - log.Fatal("CLI configuration error", "error", err) - } + // For help commands or when help flag is set, we don't want to show the error + if !errors.Is(err, config.NotFound) { + log.Fatal("CLI configuration error", "error", err) + } else if !isHelpRequested { + log.Warn("CLI configuration issue", "error", err) + }🧰 Tools
🪛 GitHub Check: golangci
[failure] 63-63: [golangci] cmd/root.go#L63
early-return: if c { ... } else { ... log.Fatal() } can be simplified to if !c { ... log.Fatal() } ... (revive)
[failure] 69-69: [golangci] cmd/root.go#L69
deep-exit: calls to log.Fatal only in main() or init() functions (revive)
69-69: Avoid log.Fatal outside main() or init() functions.Using
log.Fatalin non-main/init functions can make testing difficult as it terminates the program immediately.Consider propagating the error back to the caller instead and handle the termination at a higher level.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 69-69: [golangci] cmd/root.go#L69
deep-exit: calls to log.Fatal only in main() or init() functions (revive)pkg/logger/logger.go (1)
161-163: Add period to comment and consider moving constant.The comment for
FilePermDefaultshould end with a period as noted by the static analysis.-// FilePermDefault is the default permission for log files (0644 in octal) TODO: refactor this later +// FilePermDefault is the default permission for log files (0644 in octal). TODO: refactor this later.Also, consider moving this constant to a more appropriate location if it might be used elsewhere in the codebase.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 161-161: [golangci] pkg/logger/logger.go#L161
Comment should end in a period (godot)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/root.go(4 hunks)pkg/logger/logger.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
cmd/root.go
[failure] 63-63: [golangci] cmd/root.go#L63
early-return: if c { ... } else { ... log.Fatal() } can be simplified to if !c { ... log.Fatal() } ... (revive)
[failure] 69-69: [golangci] cmd/root.go#L69
deep-exit: calls to log.Fatal only in main() or init() functions (revive)
pkg/logger/logger.go
[failure] 161-161: [golangci] pkg/logger/logger.go#L161
Comment should end in a period (godot)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
cmd/root.go (3)
91-106: Well-structured logger setup function.The new
setupLoggerfunction nicely encapsulates the logger initialization logic, with good error handling that defaults to a sensible configuration (Info level on stderr) when initialization fails.
108-124: Good log level mapping implementation.The
mapToCharmLevelfunction clearly maps internal log levels to Charmbracelet log levels with appropriate fallbacks. The handling ofLogLevelTracemapping tolog.DebugLevelwith a clear comment about the limitation is particularly good.
126-154: Thorough output configuration with clear comments.The
configureLogOutputfunction handles all output destinations well, including standard streams and custom files. The comments explaining the need to register files for cleanup are informative.pkg/logger/logger.go (5)
39-72: Good refactoring of logger initialization.The renamed
InitializeLoggerfunction implements a cleaner approach to logger setup with proper error handling for custom log files.
74-81: Function signature improvement.Good change to accept a pointer to
schema.AtmosConfigurationrather than passing by value, which is more efficient for larger structs.
156-159: Clear helper function for log file determination.The
shouldUseCustomLogFilefunction makes the code more readable by extracting this logic into a well-named function.
164-171: Good abstraction for file opening.The
openLogFilefunction properly handles file opening with appropriate flags and error wrapping.
181-208: Consistent logging implementation.All logging methods now consistently use both the Charmbracelet logger and the original colored output. The special handling for Trace level (which doesn't exist in Charmbracelet) is well implemented.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/logger/logger.go (1)
161-163: Address TODO comment in the codeThe code contains a TODO comment about refactoring the
FilePermDefaultconstant. Consider addressing this now to avoid technical debt.-// FilePermDefault is the default permission for log files (0644 in octal). TODO: refactor this later. -const FilePermDefault = 0o644 +// FilePermDefault defines standard permission (0644) for log files +const FilePermDefault = 0o644
🛑 Comments failed to post (2)
cmd/root.go (2)
130-158: 🛠️ Refactor suggestion
File descriptor leakage risk in configureLogOutput
While the comments mention the importance of closing file descriptors, there's no actual cleanup mechanism implemented for the custom log file.
Consider registering the file with a cleanup handler:
// Handle custom log file (anything not a standard stream) customFile, err := os.OpenFile(logFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, config.StandardFilePermissions) if err != nil { log.Error("Failed to open log file, using stderr", "error", err) log.SetOutput(os.Stderr) return } +// Register file to be closed at program exit +utils.RegisterCleanupHandler(func() { + if err := customFile.Close(); err != nil { + fmt.Fprintf(os.Stderr, "Error closing log file: %v\n", err) + } +}) log.SetOutput(customFile)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// configureLogOutput sets up the output destination for the global logger. func configureLogOutput(logFile string) { // Handle standard output destinations switch logFile { case "/dev/stderr": log.SetOutput(os.Stderr) return case "/dev/stdout", "": log.SetOutput(os.Stdout) return case "/dev/null": log.SetOutput(io.Discard) return } // Handle custom log file (anything not a standard stream) customFile, err := os.OpenFile(logFile, os.O_WRONLY|os.O_APPEND|os.O_CREATE, config.StandardFilePermissions) if err != nil { log.Error("Failed to open log file, using stderr", "error", err) log.SetOutput(os.Stderr) return } // Register file to be closed at program exit utils.RegisterCleanupHandler(func() { if err := customFile.Close(); err != nil { fmt.Fprintf(os.Stderr, "Error closing log file: %v\n", err) } }) log.SetOutput(customFile) }
65-75:
⚠️ Potential issueAvoid os.Exit outside of main or init functions
The current code calls
os.Exitdirectly within a function, which violates thedeep-exitrule. This practice can prevent proper cleanup of resources and make testing harder.Consider refactoring this to return an error instead:
if errors.Is(err, config.NotFound) { // For help commands or when help flag is set, we don't want to show the error if !isHelpRequested { log.Warn("CLI configuration issue", "error", err) } return } log.Error("CLI configuration error", "error", err) -os.Exit(1) +return fmt.Errorf("CLI configuration error: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if errors.Is(err, config.NotFound) { // For help commands or when help flag is set, we don't want to show the error if !isHelpRequested { log.Warn("CLI configuration issue", "error", err) } return } log.Error("CLI configuration error", "error", err) return fmt.Errorf("CLI configuration error: %w", err)🧰 Tools
🪛 GitHub Check: golangci
[failure] 75-75: [golangci] cmd/root.go#L75
deep-exit: calls to os.Exit only in main() or init() functions (revive)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/root.go (2)
116-132: Refine 'Off' log level handling.
Using 'FatalLevel+1' for 'Off' is clever. Alternatively, consider discarding output to avoid potential confusion.
134-157: Ensure file closure.
You're correct that runtime closes files, but explicitly closing them is tidier. Consider adding a global teardown method for best practice.
🛑 Comments failed to post (1)
cmd/root.go (1)
211-211: 🛠️ Refactor suggestion
Question about repeating InitCliConfig.
You call InitCliConfig again. Evaluate merging with earlier config logic or confirming necessity for double initialization.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/logger/charmbracelet.go (1)
67-73: Consider revising the warning style color combination.The warning style uses pink background with dark gray foreground, which might not provide enough contrast compared to the error style. Consider using a different background color like yellow or orange to distinguish it more clearly from error messages.
styles.Levels[log.WarnLevel] = lipgloss.NewStyle(). SetString(warnLevelLabel). Padding(paddingVertical, paddingHorizontal, paddingVertical, paddingHorizontal). - Background(lipgloss.Color(theme.ColorPink)). + Background(lipgloss.Color(theme.ColorYellow)). Foreground(lipgloss.Color(theme.ColorDarkGray))
🛑 Comments failed to post (2)
pkg/logger/test_logger.go (2)
30-37:
⚠️ Potential issueUnused test functions should be moved to test files.
These functions are flagged as unused by static analysis. They appear to be demonstration code that should be moved to proper test files or example files.
Consider moving these functions to a file with the
_test.gosuffix, or convert them to proper examples with theExampleprefix and appropriate documentation.-func testIndividualLogFunctions() { +func ExampleLoggerFunctions() { Info("This is an info message") Debug("This is a debug message with context", KeyComponent, "station", KeyDuration, "500ms") Warn("This is a warning message", KeyStack, "prod-ue1") Error("Whoops! Something went wrong", KeyError, "kitchen on fire", KeyComponent, "weather") time.Sleep(500 * time.Millisecond) + // Output: + // This is an info message + // This is a debug message with context + // This is a warning message + // Whoops! Something went wrong }Also applies to: 39-52, 54-67
🧰 Tools
🪛 golangci-lint (1.62.2)
30-30: func
testIndividualLogFunctionsis unused(unused)
🪛 GitHub Check: golangci
[failure] 30-30: [golangci] pkg/logger/test_logger.go#L30
functestIndividualLogFunctionsis unused (unused)
19-28:
⚠️ Potential issueRemove main function from non-main package.
This file contains a
mainfunction but is part of the logger package, not a main package. This will cause build issues and has been flagged by static analysis.Consider converting this file to a proper example or test file by:
- Either renaming it to have the
_example.gosuffix if it's meant as an example- Or moving the code to a proper test file with the
_test.gosuffix-func main() { - // Test individual log functions - testIndividualLogFunctions() - - // Test complete Logger struct - testLoggerStruct() - - // Test direct charmbracelet logger - testCharmLogger() -} +// Example usage of logger functionality +// This can be moved to a proper test or example file📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Example usage of logger functionality // This can be moved to a proper test or example file🧰 Tools
🪛 golangci-lint (1.62.2)
19-19: func
mainis unused(unused)
🪛 GitHub Check: golangci
[failure] 19-19: [golangci] pkg/logger/test_logger.go#L19
funcmainis unused (unused)
what
New Style for atmos logger with theme
why
references
Summary by CodeRabbit
New Features
Refactor
Tests
Chores