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 logger initialization across CLI commands by changing the logger creation call to pass a pointer to the Atmos configuration. Additionally, a new logging package leveraging the Charmbracelet library is introduced with corresponding functions and tests added to support styled logging output. The core logger is updated to integrate Charmbracelet, with added support for custom log files and improved error handling. A new file permission constant and test utilities for logger functionalities are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI_Command as CLI Command
participant LoggerInit as Logger Initialization
participant Logger as Logger Instance
CLI_Command->>LoggerInit: Call NewLoggerFromCliConfig(&atmosConfig)
LoggerInit->>Logger: Create and configure logger (with Charmbracelet)
Logger-->>LoggerInit: Return logger instance or error
LoggerInit-->>CLI_Command: Return logger instance or propagate error
CLI_Command->>Logger: Invoke logger methods (Info, Debug, etc.)
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🪛 golangci-lint (1.62.2)pkg/logger/test_logger.go[error] 19-19: func (unused) [error] 30-30: func (unused) [error] 39-39: func (unused) [error] 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 (1)
✨ 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)
This reverts commit 15acbf6.
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 (3)
pkg/schema/schema.go (1)
82-86: Add a period to the YAMLSettings comment.The YAMLSettings struct is a great addition for controlling YAML output formatting. To adhere to Go conventions, the comment should end with a period.
-// YAMLSettings defines configuration for YAML output formatting +// YAMLSettings defines configuration for YAML output formatting.🧰 Tools
🪛 GitHub Check: golangci
[failure] 82-82: [golangci] pkg/schema/schema.go#L82
Comment should end in a period (godot)internal/exec/file_utils.go (2)
62-77: Good conditional handling for different output scenarios.The implementation correctly handles both cases with and without configuration, providing appropriate fallbacks.
This block has high complexity according to static analysis. Consider extracting the printing logic to a separate helper function to reduce nesting.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 62-62: [golangci] internal/exec/file_utils.go#L62
if file == ""has complex nested blocks (complexity: 8) (nestif)
38-38: Simplify variable declaration.The type declaration for
actualDatais redundant since it can be inferred from the right-hand side.- var actualData any = data + var actualData = data🧰 Tools
🪛 GitHub Check: golangci
[failure] 38-38: [golangci] internal/exec/file_utils.go#L38
var-declaration: should omit type any from declaration of var actualData; it will be inferred from the right-hand side (revive)
🛑 Comments failed to post (2)
internal/exec/file_utils.go (1)
80-80: 🛠️ Refactor suggestion
Extract file permission constant.
The magic number
0o644for file permissions should be extracted to a named constant.- err := u.WriteToFileAsYAML(file, actualData, 0o644) + const DefaultFilePermissions = 0o644 + err := u.WriteToFileAsYAML(file, actualData, DefaultFilePermissions)Consider adding this constant at the package level for reuse with other file operations.
📝 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.const DefaultFilePermissions = 0o644 err := u.WriteToFileAsYAML(file, actualData, DefaultFilePermissions)🧰 Tools
🪛 GitHub Check: golangci
[failure] 80-80: [golangci] internal/exec/file_utils.go#L80
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)pkg/utils/yaml_utils.go (1)
69-89: 🛠️ Refactor suggestion
Optimize large parameter passing.
The function accepts a large struct by value, which can impact performance.
-func PrintAsYAMLToFileDescriptor(atmosConfig schema.AtmosConfiguration, data any) error { +func PrintAsYAMLToFileDescriptor(atmosConfig *schema.AtmosConfiguration, data any) error { - indent := GetYAMLIndent(&atmosConfig) + indent := GetYAMLIndent(atmosConfig) - highlighted, err := HighlightCodeWithConfig(y, atmosConfig, "yaml") + highlighted, err := HighlightCodeWithConfig(y, *atmosConfig, "yaml")Also update any call sites to pass a pointer instead of the value.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 70-70: [golangci] pkg/utils/yaml_utils.go#L70
hugeParam: atmosConfig is heavy (5776 bytes); consider passing it by pointer (gocritic)
pkg/pro/api_client_test.go
Outdated
| @@ -114,7 +114,7 @@ func TestUnlockStack(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestUnlockStack_Error(t *testing.T) { | |||
| mockLogger, err := logger.NewLogger("test", "/dev/stdout") | |||
| mockLogger, err := logger.InitializeLogger("test", "/dev/stdout") | |||
There was a problem hiding this comment.
Use /dev/stderr for logs. Not that it matters here, what happened to us was for a example atmos configuration we used /dev/stdout, that got copied over and over again so that it became the default but it was always wrong. So everywhere update this to stderr.
| // configureLogLevelStyles configures the styles for different log levels. | ||
| func configureLogLevelStyles(styles *log.Styles, paddingVertical, paddingHorizontal int) { | ||
| const ( | ||
| errorLevelLabel = "ERROR" |
There was a problem hiding this comment.
| errorLevelLabel = "ERROR" | |
| errorLevelLabel = "ERRO" |
| helperLogger = GetCharmLogger() | ||
| } | ||
|
|
||
| // GetCharmLogger returns a pre-configured Charmbracelet logger with Atmos styling. |
There was a problem hiding this comment.
| // GetCharmLogger returns a pre-configured Charmbracelet logger with Atmos styling. | |
| // GetAtmosLogger returns a pre-configured Charmbracelet logger with Atmos styling. |
| assert.NotNil(t, styles.Keys["stack"], "stack key should have styling") | ||
| } | ||
|
|
||
| func ExampleGetCharmLogger() { |
| @@ -20,7 +21,7 @@ const ( | |||
| LogLevelWarning LogLevel = "Warning" | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Note, why don't we have LogLevelError above?
|
|
||
| // Set log level | ||
| charmLevel := log.InfoLevel | ||
| switch logLevel { |
pkg/logger/logger.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func NewLoggerFromCliConfig(cfg schema.AtmosConfiguration) (*Logger, error) { | ||
| // InitializeLoggerFromCliConfig creates a logger based on Atmos CLI configuration. | ||
| func InitializeLoggerFromCliConfig(cfg *schema.AtmosConfiguration) (*Logger, error) { |
There was a problem hiding this comment.
The NewX naming convention is the standard way to create and return a new instance of a type.
pkg/logger/logger.go
Outdated
| } | ||
|
|
||
| func NewLogger(logLevel LogLevel, file string) (*Logger, error) { | ||
| func InitializeLogger(logLevel LogLevel, file string) (*Logger, error) { |
There was a problem hiding this comment.
The NewX naming convention is the standard way to create and return a new instance of a type.
There was a problem hiding this comment.
I guess i'm confused now between this and the functions above that were called GetX but should also have been NewX and these here, without investing too much mental effort.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
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 (11)
pkg/logger/charmbracelet.go (11)
11-11: Consider documenting the custom TraceLevel constant.The TraceLevel constant is defined but lacks documentation that explains its purpose and relationship to standard log levels.
+// TraceLevel is a custom log level below DebugLevel for more granular tracing information. const TraceLevel log.Level = log.DebugLevel - 1
13-17: Consider making the helper logger configurable after initialization.The global logger is initialized at package level with fixed configuration. Consider adding a way to update it after initialization for more flexibility in testing or different runtime environments.
var helperLogger *log.Logger func init() { helperLogger = GetCharmLogger() } + +// SetHelperLogger allows replacing the default helper logger with a custom configured one. +func SetHelperLogger(logger *log.Logger) { + helperLogger = logger +}
35-36: Improve documentation for clarity.The function documentation should be a complete sentence for consistency.
-// getAtmosLogStyles returns custom styles for the Charmbracelet logger using Atmos theme colors. +// getAtmosLogStyles returns custom styles for the Charmbracelet logger using Atmos theme colors.
50-51: Update function documentation to complete sentence.For consistency with standard Golang documentation practices.
-// configureLogLevelStyles configures the styles for different log levels. +// configureLogLevelStyles configures the styles for different log levels.
96-97: Update function documentation to complete sentence.For consistency with Golang documentation standards.
-// configureKeyStyles configures the styles for different log keys. +// configureKeyStyles configures the styles for different log keys.
105-105: Inconsistent comment compared to other similar comments.The comment doesn't match the format of other key style comments. Also, there's a discrepancy between 'err' in the comment and 'error' in the code.
-// Custom style for 'err' key +// Custom style for 'error' key.
109-109: Update comment format to use complete sentences.For consistency with Golang documentation standards.
-// Custom style for 'component' key +// Custom style for 'component' key.
112-112: Update comment format to use complete sentences.For consistency with Golang documentation standards.
-// Custom style for 'stack' key +// Custom style for 'stack' key.
115-115: Update comment format to use complete sentences.For consistency with Golang documentation standards.
-// Custom style for 'duration' key +// Custom style for 'duration' key.
119-122: Add a log level parameter to the helper functions.Consider adding an optional log level parameter to these helper functions, allowing for more flexibility when logging.
// Error logs an error message with context. func Error(message string, keyvals ...interface{}) { helperLogger.Error(message, keyvals...) }
144-147: Document that Fatal exits the program.The Fatal function documentation should explicitly mention that it terminates the program with exit code 1, as this is a critical behavior.
-// Fatal logs an error message and exits with status code 1. +// Fatal logs an error message and exits the program with status code 1. func Fatal(message string, keyvals ...interface{}) { helperLogger.Fatal(message, keyvals...) }
🛑 Comments failed to post (2)
pkg/logger/charmbracelet.go (2)
15-25: 💡 Verification agent
🧩 Analysis chain
Update helperLogger initialization to use new function name.
If you rename GetCharmLogger to NewAtmosLogger, don't forget to update the initialization in the init function as well.
🏁 Script executed:
#!/bin/bash # Check for other references to GetCharmLogger that would need updating grep -r "GetCharmLogger" --include="*.go" .Length of output: 1105
Action Required: Update function name consistently
The initialization in
pkg/logger/charmbracelet.gois still callingGetCharmLogger(). If you’ve renamed this function toNewAtmosLogger(), please update the call in theinitfunction accordingly. Also, review and update all related references (including in documentation comments and tests such as inpkg/logger/charmbracelet_test.go,pkg/logger/logger.go, andpkg/logger/test_logger.go) to ensure consistency across the codebase.
139-142: 💡 Verification agent
🧩 Analysis chain
Consider using a custom level constant in the logger initialization.
The Trace function calls Log with TraceLevel, but the logger isn't explicitly configured to handle this custom level. Consider registering the custom level with the logger at initialization.
🏁 Script executed:
#!/bin/bash # Check if the Charmbracelet log package supports registering custom levels cat << EOF > /tmp/check_custom_level.go package main import ( "fmt" "os" log "github.com/charmbracelet/log" ) const CustomLevel log.Level = log.DebugLevel - 1 func main() { logger := log.New(os.Stdout) // Check if there's a method to register custom levels fmt.Println("Checking for methods to register custom log levels:") // This is just a simulation as we can't actually reflect on the methods fmt.Println("Available methods (simulated):", "SetLevel, SetReportCaller, SetReportTimestamp, SetStyles") logger.Log(CustomLevel, "This is a custom level message") fmt.Println("Does the message appear with the correct level indicator?") } EOF go mod init checklogger go get github.com/charmbracelet/log go run /tmp/check_custom_level.goLength of output: 1601
Action Required: Explicitly Register the Custom Trace Level
While the
Tracefunction logs messages usingTraceLevel, the logger's initialization isn’t currently set up to recognize this custom level. Our verification confirms that the Charmbracelet log package exposes configuration methods (such asSetStyles) but does not automatically handle custom levels. To ensure that trace logs are rendered with the proper style and behavior, please update the logger initialization to explicitly register or configureTraceLevel.
- File:
pkg/logger/charmbracelet.go(lines 139–142)- Suggestion: In the logger’s setup, map or register
TraceLevelusing the available configuration methods (e.g.,SetStyles) so that the custom level is recognized and applied correctly.
…nto feature/DEV-3010-atmos-logger
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: 5
🔭 Outside diff range comments (1)
pkg/logger/test_logger.go (1)
1-68: 🛠️ Refactor suggestionReview overall file purpose and implementation.
This file combines a number of test functions but isn't structured as a proper Go test file. It appears to be a manual test utility rather than an automated test. Consider:
- Renaming the file to
logger_test.goif it's meant to be a test file- Converting all functions to proper Go test functions (func TestXxx(t *testing.T))
- Adding assertions to verify the expected behavior instead of just executing functions
- Removing sleep calls which slow down tests unnecessarily
This approach will make the tests more reliable, automatically runnable with the Go test command, and more maintainable.
🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 19-19: func
mainis unused(unused)
[error] 30-30: func
testIndividualLogFunctionsis unused(unused)
[error] 39-39: func
testLoggerStructis unused(unused)
[error] 54-54: func
testCharmLoggeris unused(unused)
🪛 GitHub Check: golangci
[failure] 19-19: [golangci] pkg/logger/test_logger.go#L19
funcmainis unused (unused)
[failure] 30-30: [golangci] pkg/logger/test_logger.go#L30
functestIndividualLogFunctionsis unused (unused)
[failure] 39-39: [golangci] pkg/logger/test_logger.go#L39
functestLoggerStructis unused (unused)
[failure] 54-54: [golangci] pkg/logger/test_logger.go#L54
functestCharmLoggeris unused (unused)
🛑 Comments failed to post (5)
pkg/logger/test_logger.go (5)
1-7: 🛠️ Refactor suggestion
Add proper imports for Go testing.
Since this file appears to be for testing, you should add the testing package import.
package logger import ( "errors" "time" + "testing" )📝 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.package logger import ( "errors" "time" "testing" )
54-67:
⚠️ Potential issueConvert to proper Go test function.
This function is unused and should be converted to a standard Go test function.
-func testCharmLogger() { +func TestCharmLogger(t *testing.T) { charmLogger := GetCharmLogger() charmLogger.SetTimeFormat(time.Kitchen) charmLogger.Info("Processing component", KeyComponent, "station", KeyStack, "dev-ue1") charmLogger.Debug("Found configuration", KeyPath, "/stacks/deploy/us-east-1/dev/station.yaml") charmLogger.Warn("Component configuration outdated", KeyComponent, "weather", "lastUpdated", "90 days ago") charmLogger.Error("Failed to apply changes", KeyError, "validation failed", KeyComponent, "weather", KeyDetails, "required variables missing", KeyStack, "dev-ue1") }📝 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.func TestCharmLogger(t *testing.T) { charmLogger := GetCharmLogger() charmLogger.SetTimeFormat(time.Kitchen) charmLogger.Info("Processing component", KeyComponent, "station", KeyStack, "dev-ue1") charmLogger.Debug("Found configuration", KeyPath, "/stacks/deploy/us-east-1/dev/station.yaml") charmLogger.Warn("Component configuration outdated", KeyComponent, "weather", "lastUpdated", "90 days ago") charmLogger.Error("Failed to apply changes", KeyError, "validation failed", KeyComponent, "weather", KeyDetails, "required variables missing", KeyStack, "dev-ue1") }🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 54-54: func
testCharmLoggeris unused(unused)
🪛 GitHub Check: golangci
[failure] 54-54: [golangci] pkg/logger/test_logger.go#L54
functestCharmLoggeris unused (unused)
19-28:
⚠️ Potential issueRemove unused
mainfunction from package.The
mainfunction can't be executed in a non-main package. This appears to be a manual test runner, but Go has a built-in testing framework that should be used instead.-func main() { - // Test individual log functions - testIndividualLogFunctions() - - // Test complete Logger struct - testLoggerStruct() - - // Test direct charmbracelet logger - testCharmLogger() -}If you need to test these functions, convert them to proper Go tests or create an example package.
📝 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.🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 19-19: func
mainis unused(unused)
🪛 GitHub Check: golangci
[failure] 19-19: [golangci] pkg/logger/test_logger.go#L19
funcmainis unused (unused)
39-52:
⚠️ Potential issueConvert to proper Go test function and improve error handling.
This function should follow Go's testing conventions and needs better error handling.
-func testLoggerStruct() { +func TestLoggerStruct(t *testing.T) { atmosLogger, err := NewLogger(LogLevelTrace, "") if err != nil { - panic(err) + t.Fatalf("Failed to create logger: %v", err) } atmosLogger.Trace("This is a trace message") atmosLogger.Debug("This is a debug message") atmosLogger.Info("This is an info message") atmosLogger.Warning("This is a warning message") atmosLogger.Error(ErrTest) - - time.Sleep(500 * time.Millisecond) }📝 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.func TestLoggerStruct(t *testing.T) { atmosLogger, err := NewLogger(LogLevelTrace, "") if err != nil { t.Fatalf("Failed to create logger: %v", err) } atmosLogger.Trace("This is a trace message") atmosLogger.Debug("This is a debug message") atmosLogger.Info("This is an info message") atmosLogger.Warning("This is a warning message") atmosLogger.Error(ErrTest) }🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 39-39: func
testLoggerStructis unused(unused)
🪛 GitHub Check: golangci
[failure] 39-39: [golangci] pkg/logger/test_logger.go#L39
functestLoggerStructis unused (unused)
30-37:
⚠️ Potential issueConvert to proper Go test function.
This function is flagged as unused. Convert it to a standard Go test function to make it part of the test suite.
-func testIndividualLogFunctions() { +func TestIndividualLogFunctions(t *testing.T) { 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) + // Remove sleep as it slows down tests unnecessarily }Don't forget to import the
testingpackage at the top of the 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.package logger import ( "testing" "time" // assuming time is used elsewhere in the file; remove if not needed ) func TestIndividualLogFunctions(t *testing.T) { 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") // Remove sleep as it slows down tests unnecessarily }🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 30-30: func
testIndividualLogFunctionsis unused(unused)
🪛 GitHub Check: golangci
[failure] 30-30: [golangci] pkg/logger/test_logger.go#L30
functestIndividualLogFunctionsis unused (unused)
| // isLevelEnabled checks if a given log level should be enabled based on the logger's current level | ||
| // shouldUseCustomLogFile returns true if a custom log file should be used instead of standard streams. | ||
| func shouldUseCustomLogFile(file string) bool { | ||
| return file != "" && file != "/dev/stdout" && file != "/dev/stderr" |
| @@ -46,6 +45,8 @@ func TestNewLogger(t *testing.T) { | |||
| assert.NotNil(t, logger) | |||
| assert.Equal(t, LogLevelDebug, logger.LogLevel) | |||
| assert.Equal(t, "/dev/stdout", logger.File) | |||
There was a problem hiding this comment.
Actually, let's add a warning somewhere in our code if using /dev/stdout because that will break downstream tools like our github actions. And recommend using /dev/stderr
what
New Style for atmos logger with theme
why
references
Summary by CodeRabbit
New Features
Refactor