Style Atmos Logger with Theme #1135
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the logging system throughout the codebase. The changes introduce a new logger type, AtmosLogger, and update log level handling by adding an "Error" case and a new log level (AtmosTraceLevel). Logger initialization is now performed with a pointer to the configuration, and tests have been updated accordingly. Changes affect command setup, CLI argument parsing, and the API client, ensuring that logger instances and methods are now consistent across modules. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant CmdRoot
participant LoggerFactory
participant GlobalLogger
CLI ->> CmdRoot: Calls setupLogger()
CmdRoot ->> LoggerFactory: Invokes NewAtmosLogger(writer)
LoggerFactory -->> CmdRoot: Returns AtmosLogger instance
CmdRoot ->> GlobalLogger: Sets default logger via SetDefault(AtmosLogger)
Assessment against linked issues
Possibly related PRs
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)cmd/root.go (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (3)
✨ 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: 2
🧹 Nitpick comments (4)
cmd/root.go (1)
114-115: Use of SetDefault for a global logger.
Be mindful of potential concurrency scenarios; otherwise, this is fine.pkg/logger/logger_test.go (2)
143-155: Error test uses fmt.Errorf, flagged by static analysis.
Consider using a package-level error if you want to avoid dynamic errors:- err := fmt.Errorf("This is an error") + var errTest = errors.New("This is an error")🧰 Tools
🪛 GitHub Check: golangci
[failure] 151-151: [golangci] pkg/logger/logger_test.go#L151
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("This is an error")" (err113)
156-168: File logger error test likewise uses a dynamic error.
Follow the same approach for consistency if avoiding dynamic errors:- err2 := fmt.Errorf("This is a file error") + var errFileTest = errors.New("This is a file error")🧰 Tools
🪛 GitHub Check: golangci
[failure] 164-164: [golangci] pkg/logger/logger_test.go#L164
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("This is a file error")" (err113)pkg/logger/logger.go (1)
45-73: File writer logic and styled logger creation.
Addresses writing to various outputs but does not track file closure. Also note the static analysis suggestion about magic number 0o644:- writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o644) + const filePermissions = 0o644 + writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, filePermissions)🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 52-52: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
[failure] 52-52: [golangci] pkg/logger/logger.go#L52
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
🛑 Comments failed to post (2)
cmd/root.go (1)
117-131:
⚠️ Potential issueInconsistent handling of the 'Off' level compared to NewLogger.
Here, “Off” sets the log level tomath.MaxInt32, but the logger package useslog.ErrorLevelfor “Off.” This discrepancy could cause unpredictable logging behavior.- case "Off": - styledLog.SetLevel(math.MaxInt32) + case "Off": + styledLog.SetLevel(log.ErrorLevel)📝 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.// Set the appropriate log level based on configuration. switch atmosConfig.Logs.Level { case "Trace": styledLog.SetLevel(logger.AtmosTraceLevel) case "Debug": styledLog.SetLevel(log.DebugLevel) case "Info": styledLog.SetLevel(log.InfoLevel) case "Warning": styledLog.SetLevel(log.WarnLevel) case "Off": styledLog.SetLevel(log.ErrorLevel) default: styledLog.SetLevel(log.InfoLevel) }pkg/logger/logger.go (1)
145-161: 🛠️ Refactor suggestion
SetLogLevel logic.
There is a mismatch between “Off” here (mapped to ErrorLevel) andcmd/root.go(set to math.MaxInt32). A single approach ensures consistent suppression of output.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/logger/logger.go (1)
118-135: 🛠️ Refactor suggestionDuplicate file handling logic should be centralized.
This code replicates the file handling logic that now exists in the
NewLoggerfunction. Since we're now using the styled logger, we should update this method to use it or remove it if it's no longer needed.Consider refactoring this to use the
StyledLoggerfor all logging operations.🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 118-118: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
[error] 131-131: add-constant: string literal "%s\n" appears, at least, 4 times, create a named constant for it
(revive)
♻️ Duplicate comments (1)
pkg/logger/logger.go (1)
45-56:⚠️ Potential issueReconsider default log output destination.
The current implementation sets
os.Stdoutas the default output writer. This could potentially break downstream commands that rely onatmosoutput, as JSON output and logs cannot coexist onstdoutwithout causing parsing issues.Consider making
/dev/stderrthe default to maintain backward compatibility and prevent unintended side effects.-var writer io.Writer = os.Stdout +var writer io.Writer = os.Stderr🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 52-52: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
[failure] 52-52: [golangci] pkg/logger/logger.go#L52
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
🧹 Nitpick comments (5)
pkg/logger/logger.go (5)
26-27: Consider using a relative definition for AtmosTraceLevel.The
AtmosTraceLevelis currently defined as a hard-coded-5with a comment indicating that DebugLevel is-4. A more maintainable approach would be to define it relative tolog.DebugLevel:-const AtmosTraceLevel log.Level = -5 // DebugLevel is -4 +const AtmosTraceLevel log.Level = log.DebugLevel - 1This makes the relationship clearer and avoids issues if the underlying library changes level values.
52-52: Create a named constant for file permission value.Avoid using magic numbers like '0o644' directly in the code. Create a named constant to improve readability and maintainability.
+const defaultLogFilePermissions = 0o644 -writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o644) +writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, defaultLogFilePermissions)🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 52-52: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
[failure] 52-52: [golangci] pkg/logger/logger.go#L52
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
59-59: Rename logger creation function to match intention.The function name should reflect that this is specifically an Atmos-themed logger, as noted in previous feedback.
-styledLogger := NewStyledLogger(writer) +styledLogger := NewAtmosLogger(writer)
166-167: Revisit conditional Error logging.The condition
l.LogLevel != LogLevelOffmight not be necessary since theStyledLoggeralready regulates output based on its configured level (set inSetLogLevel). Consider simplifying this:-if err != nil && l.LogLevel != LogLevelOff { +if err != nil { l.StyledLogger.Error("Error occurred", "error", err) }
223-224: Update function name and documentation to better reflect purpose.Rename the function and update its documentation to clearly indicate this is an Atmos-specific logger implementation:
-// NewStyledLogger creates a new styled Charmbracelet logger using Atmos theme colors. -func NewStyledLogger(writer io.Writer) *log.Logger { +// NewAtmosLogger creates a new Charmbracelet logger styled with Atmos theme colors. +func NewAtmosLogger(writer io.Writer) *log.Logger {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/logger/logger.go(5 hunks)pkg/logger/logger.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/logger/logger.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/logger/logger.go
[error] 52-52: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
pkg/logger/logger.go
[failure] 52-52: [golangci] pkg/logger/logger.go#L52
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/logger/logger.go (2)
234-234: LGTM - Great implementation of the trace level styling.The implementation of the trace level styling is clean and consistent with the other log levels. The use of the Atmos theme colors provides a cohesive visual experience.
240-248: Strong approach to style organization.Using a map to concisely declare key styles is a great pattern that improves maintainability. This makes it easy to add or modify key styles in the future.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/logger/logger.go (3)
26-27: Consider defining AtmosTraceLevel relative to log.DebugLevelThe constant is correctly defined, but it would be more maintainable to define it relative to the DebugLevel in case that value changes in future library updates.
-const AtmosTraceLevel log.Level = -5 // DebugLevel is -4 +const AtmosTraceLevel log.Level = log.DebugLevel - 1
166-168: Consider simplifying error logging conditionThe condition
l.LogLevel != LogLevelOffmay be redundant since you have theisLevelEnabledfunction that handles log level checking consistently.-func (l *Logger) Error(err error) { - if err != nil && l.LogLevel != LogLevelOff { - l.StyledLogger.Error("Error occurred", "error", err) - } -} +func (l *Logger) Error(err error) { + if err != nil && l.isLevelEnabled(LogLevelWarning) { + l.StyledLogger.Error("Error occurred", "error", err) + } +}This would make the error handling consistent with other logging methods.
223-255: Well-implemented styled loggerThe styled logger implementation is well-structured and makes good use of the theme colors from the theme package. The styling is consistent and provides good visual differentiation between log levels.
Consider renaming the function to
NewAtmosLoggerbased on previous review comments to maintain consistency with the theme of the application.-// NewStyledLogger creates a new styled Charmbracelet logger using Atmos theme colors. -func NewStyledLogger(writer io.Writer) *log.Logger { +// NewAtmosLogger creates a new Charmbracelet logger styled with Atmos theme colors. +func NewAtmosLogger(writer io.Writer) *log.Logger {And update all references to this function accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/logger/logger.go(5 hunks)pkg/logger/logger.go(1 hunks)pkg/logger/logger.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/logger/logger.go
- pkg/logger/logger.go
🧰 Additional context used
🪛 GitHub Check: golangci
pkg/logger/logger.go
[failure] 59-59: [golangci] pkg/logger/logger.go#L59
undefined: NewStyledLogger) (typecheck)
[failure] 59-59: [golangci] pkg/logger/logger.go#L59
undefined: NewStyledLogger) (typecheck)
[failure] 59-59: [golangci] pkg/logger/logger.go#L59
undefined: NewStyledLogger) (typecheck)
[failure] 59-59: [golangci] pkg/logger/logger.go#L59
undefined: NewStyledLogger
[failure] 59-59: [golangci] pkg/logger/logger.go#L59
undefined: NewStyledLogger) (typecheck)
🪛 GitHub Check: Build (macos-latest, macos)
pkg/logger/logger.go
[failure] 59-59:
undefined: NewStyledLogger
🪛 GitHub Check: Build (ubuntu-latest, linux)
pkg/logger/logger.go
[failure] 59-59:
undefined: NewStyledLogger
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/logger/logger.go (3)
45-56: Writer selection logic looks goodThe code correctly handles different output destinations, ensuring logs can be written to stdout, stderr, or a file as needed.
147-160: Log level mapping looks goodThe implementation correctly maps the custom log levels to the Charmbracelet log levels, including the new trace level.
235-238: Good attention to error stylingThe special styling for error values enhances readability and helps important errors stand out visually.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/logger/logger_test.go (1)
151-151: Address static analysis warning about dynamic errors.The static analysis tool suggests using wrapped static errors instead of dynamic errors.
Consider defining static error variables at package level instead of using dynamic errors:
+var ( + testError = errors.New("This is an error") + fileError = errors.New("This is a file error") +) - err := fmt.Errorf("This is an error") + err := testError - err2 := fmt.Errorf("This is a file error") + err2 := fileErrorDon't forget to add
errorsto your imports:import ( "bytes" + "errors" "fmt" // other imports... )Also applies to: 164-164
🧰 Tools
🪛 GitHub Check: golangci
[failure] 151-151: [golangci] pkg/logger/logger_test.go#L151
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("This is an error")" (err113)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/root.go(3 hunks)pkg/logger/logger.go(5 hunks)pkg/logger/logger_test.go(2 hunks)pkg/logger/logger.go(1 hunks)pkg/logger/logger.go(1 hunks)cmd/root.go(1 hunks)pkg/logger/logger.go(6 hunks)pkg/logger/logger_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/logger/logger.go
- pkg/logger/logger.go
- cmd/root.go
- cmd/root.go
- pkg/logger/logger_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/logger/logger.go
[error] 52-52: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
pkg/logger/logger.go
[failure] 52-52: [golangci] pkg/logger/logger.go#L52
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
pkg/logger/logger_test.go
[failure] 151-151: [golangci] pkg/logger/logger_test.go#L151
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("This is an error")" (err113)
[failure] 164-164: [golangci] pkg/logger/logger_test.go#L164
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("This is a file error")" (err113)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (12)
pkg/logger/logger.go (8)
39-41: Good renaming from StyledLogger to AtmosLogger.This renaming aligns with the PR objectives to style the Atmos Logger with theme, making the code more descriptive of its purpose within the project.
58-59: Renaming correctly applied in logger creation.The comment and function call have been consistently updated to use the new AtmosLogger name.
64-72: Log level settings properly updated.All references to the logger field have been consistently renamed from StyledLogger to AtmosLogger.
76-78: Struct initialization updated correctly.The Logger struct initialization has been updated to use the new AtmosLogger field name.
147-159: Log level setting logic updated correctly.All references to the AtmosLogger in the SetLogLevel method have been properly renamed.
166-167: Error logging method updated correctly.This addresses the previous review comment by osterman asking "What about Error?", ensuring consistent renaming across all logging methods.
179-181: All logging methods consistently updated.The Trace, Debug, Info, and Warning methods have all been updated to use AtmosLogger.
Also applies to: 185-187, 191-193, 197-199
223-224: Function renamed and documentation updated.The function has been renamed from NewStyledLogger to NewAtmosLogger, and the documentation has been updated to reflect this change.
pkg/logger/logger_test.go (4)
11-11: Import updated to use charmbracelet/log.The import statement has been properly updated to use the charmbracelet/log package.
108-108:⚠️ Potential issueFix inconsistent field reference.
This line still references StyledLogger but needs to be updated to AtmosLogger.
Apply this fix:
- assert.NotNil(t, logger.StyledLogger) + assert.NotNil(t, logger.AtmosLogger)Likely an incorrect or invalid review comment.
158-162:⚠️ Potential issueUpdate logger field and constructor in file logger test.
Both the field name and constructor call need to be updated to match the renamed AtmosLogger.
Apply this fix:
fileLogger := &Logger{ LogLevel: LogLevelWarning, File: "test.log", - StyledLogger: NewStyledLogger(&buf2), + AtmosLogger: NewAtmosLogger(&buf2), }Likely an incorrect or invalid review comment.
145-149:⚠️ Potential issueUpdate logger field and constructor in Error test.
Both the field name and constructor call need to be updated to match the renamed AtmosLogger.
Apply this fix:
logger := &Logger{ LogLevel: LogLevelWarning, File: "/dev/stderr", - StyledLogger: NewStyledLogger(&buf), + AtmosLogger: NewAtmosLogger(&buf), }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/logger/logger.go (1)
46-46:⚠️ Potential issueDefault logging to stderr instead of stdout.
Setting stdout as the default writer may interfere with command output parsing. Previous discussions indicated stderr should be the default to avoid breaking commands that depend on parsing Atmos output.
- var writer io.Writer = os.Stdout + var writer io.Writer = os.StderrThis aligns with the previous recommendation that "/dev/stderr" should be the default log destination.
🧹 Nitpick comments (5)
pkg/logger/logger.go (5)
26-27: Consider deriving AtmosTraceLevel from log.DebugLevel for clearer relationship.The definition of AtmosTraceLevel as a fixed value
-5is functional but could be more maintainable by deriving it directly from the DebugLevel constant.- const AtmosTraceLevel log.Level = -5 // DebugLevel is -4 + const AtmosTraceLevel log.Level = log.DebugLevel - 1This approach makes the relationship between log levels explicit and will automatically adapt if the base levels change in the future.
44-56: Extract file permission constant for better maintainability.The magic number
0o644for file permissions should be extracted into a named constant.+// DefaultFilePermissions represents standard file permissions for log files (rw-r--r--) +const DefaultFilePermissions = 0o644 func NewLogger(logLevel LogLevel, file string) (*Logger, error) { // Determine the output writer based on file path. var writer io.Writer = os.Stdout if file == "/dev/stderr" { writer = os.Stderr } else if file != "" && file != "/dev/stdout" { // For actual files, we'll use a file writer. var err error - writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o644) + writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, DefaultFilePermissions)This will also need to be updated at line 118 where the same magic number appears.
🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 52-52: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
[failure] 52-52: [golangci] pkg/logger/logger.go#L52
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
166-168: Simplify error logging by removing redundant condition.The condition check is unnecessary since the StyledLogger already handles log level filtering based on its configured level.
- func (l *Logger) Error(err error) { - if err != nil && l.LogLevel != LogLevelOff { - l.StyledLogger.Error("Error occurred", "error", err) - } - } + func (l *Logger) Error(err error) { + if err != nil { + l.StyledLogger.Error("Error occurred", "error", err) + } + }This simplifies the code and relies on the StyledLogger's level configuration to control output.
179-182: Simplify logging methods by removing redundant level checks.All the logging methods (Trace, Debug, Info, Warning) include level checks that are redundant when using StyledLogger, which already handles level filtering.
For the Trace method:
func (l *Logger) Trace(message string) { - if l.isLevelEnabled(LogLevelTrace) { l.StyledLogger.Log(AtmosTraceLevel, message) - } }Similar changes should be applied to the Debug, Info, and Warning methods as well.
224-255: Function needs to be moved earlier in the file.The
NewStyledLoggerfunction is called at line 59 but defined much later. While Go allows this, it's better practice to define functions before they're used.Move this function definition to before line 44 where
NewLoggerstarts, to improve code readability and organization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/logger/logger.go(5 hunks)pkg/logger/logger.go(1 hunks)pkg/logger/logger.go(1 hunks)pkg/logger/logger.go(6 hunks)pkg/logger/logger.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/logger/logger.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/logger/logger.go
- pkg/logger/logger.go
- pkg/logger/logger.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/logger/logger.go
[error] 52-52: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
pkg/logger/logger.go
[failure] 52-52: [golangci] pkg/logger/logger.go#L52
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/logger/logger.go (1)
223-224: Rename function to 'NewAtmosLogger' for consistency.Based on previous feedback, this function should be renamed to explicitly reference Atmos in its name.
- // NewStyledLogger creates a new styled Charmbracelet logger using Atmos theme colors. - func NewStyledLogger(writer io.Writer) *log.Logger { + // NewAtmosLogger creates a new Charmbracelet logger styled with Atmos theme colors. + func NewAtmosLogger(writer io.Writer) *log.Logger {Also update all references to this function in the codebase, such as at line 59.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/logger/logger.go (1)
31-36: 🛠️ Refactor suggestionRemove the duplicate
LogLevelDebugentry inlogLevelOrder.Currently,
logLevelOrdercontains two identical entries ofLogLevelDebug, which is likely an oversight that can create confusion. Consider removing one occurrence to ensure clarity.var logLevelOrder = map[LogLevel]int{ LogLevelTrace: 0, LogLevelDebug: 1, - LogLevelDebug: 1, LogLevelInfo: 2, LogLevelWarning: 3, }
🧹 Nitpick comments (1)
pkg/logger/logger.go (1)
52-52: Avoid using the raw numeric file mode0o644.Defining it as a named constant or a variable makes the code more expressive and helps avoid repetition.
- writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o644) + const filePerm os.FileMode = 0o644 + writer, err = os.OpenFile(file, os.O_WRONLY|os.O_APPEND|os.O_CREATE, filePerm)Also applies to: 122-122
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/logger/logger.go(5 hunks)pkg/logger/logger.go(1 hunks)pkg/logger/logger.go(1 hunks)pkg/logger/logger.go(6 hunks)pkg/logger/logger.go(1 hunks)pkg/logger/logger.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/logger/logger.go
- pkg/logger/logger.go
- pkg/logger/logger.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/logger/logger.go
[error] 46-46: cyclomatic: function NewLogger has cyclomatic complexity 11 (> max enabled 10)
(revive)
[error] 54-54: add-constant: avoid magic numbers like '0o644', create a named constant for it
(revive)
🪛 GitHub Check: golangci
pkg/logger/logger.go
[failure] 54-54: [golangci] pkg/logger/logger.go#L54
add-constant: avoid magic numbers like '0o644', create a named constant for it (revive)
[failure] 46-46: [golangci] pkg/logger/logger.go#L46
cyclomatic: function NewLogger has cyclomatic complexity 11 (> max enabled 10) (revive)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/logger/logger.go (1)
99-106: EnsureLogLevelErroris declared before usage.
LogLevelErroris included invalidLevelsyet is not defined in your constants. Either define it in the log-level enum or remove it fromvalidLevelsto maintain consistency.
This reverts commit 58fc21e.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/logger/logger.go (2)
127-140: Include "Error" in validLevels list.The
validLevelsslice should include "Error" since it's supported in theLogLevelStringsmap.- validLevels := []string{"Trace", "Debug", "Info", "Warning", "Off"} + validLevels := []string{"Trace", "Debug", "Info", "Warning", "Error", "Off"}
139-139: Use static error pattern consistently.This line creates a dynamic error message with
fmt.Errorf, which was flagged in previous static analysis. Consider using a custom error type pattern like you did withErrUnsupportedDeviceFile.- return 0, fmt.Errorf("%w `%s`. Valid options are: %v", ErrInvalidLogLevel, logLevel, validLevels) + return 0, &InvalidLogLevelError{LogLevel: logLevel, ValidLevels: validLevels, Err: ErrInvalidLogLevel}And add this error type:
// InvalidLogLevelError represents an error when an invalid log level is provided type InvalidLogLevelError struct { LogLevel string ValidLevels []string Err error } func (e *InvalidLogLevelError) Error() string { return fmt.Sprintf("%v `%s`. Valid options are: %v", e.Err, e.LogLevel, e.ValidLevels) } func (e *InvalidLogLevelError) Unwrap() error { return e.Err }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/logger/logger.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Analyze (go)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (11)
pkg/logger/logger.go (11)
17-17: Good addition of FilePermission constant.This addresses the "magic number" issue flagged in static analysis. Using a named constant improves code readability and maintainability.
19-20: Well-defined custom log level.Good implementation of
AtmosTraceLevelas one level belowDebugLevelfor more granular logging control.
22-30: Clean log level mapping implementation.The
LogLevelStringsmap provides a clean way to map string representations to charmbracelet log levels, with proper handling of the "Off" level.
32-43: Good implementation of static errors.This addresses the previous code scanning alerts by implementing static errors instead of dynamic errors, which is a Go best practice.
45-47: Clean AtmosLogger implementation.The
AtmosLoggerstruct properly embeds the charmbracelet logger, following the composition pattern recommended in Go.
49-68: Robust log destination validation.The validation logic properly handles standard device files and provides specific warnings for stdout.
70-94: Comprehensive log writer resolution.The
getLogWriterfunction handles all common log destinations with appropriate warnings and fallbacks.
96-117: Streamlined logger creation.The refactored
NewLoggerfunction has a cleaner flow with proper validation and error handling.
152-156: Consistent error handling.The Error method properly checks the log level before logging.
193-202: Well-structured level styling function.The
createLevelStylefunction provides a clean way to create consistent level styles.
204-239: Comprehensive Atmos theme implementation.The
NewAtmosLoggerfunction provides a well-styled logger with custom colors for different log levels and key types.Consider using different colors for Error and Warning levels to improve visual distinction:
- styles.Levels[log.WarnLevel] = createLevelStyle(warnLevelLabel, theme.ColorRed) + styles.Levels[log.WarnLevel] = createLevelStyle(warnLevelLabel, theme.ColorOrange)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cmd/root.go (1)
83-100:⚠️ Potential issueAvoid closing the log file prematurely.
Placing
defer logFile.Close()insidesetupLoggerwill close the file handle once the function returns, preventing further writes. Consider keeping the file open for the application’s lifecycle:func setupLogger(atmosConfig *schema.AtmosConfiguration) { ... logFile, err := os.OpenFile(atmosConfig.Logs.File, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o666) if err != nil { log.Fatal("Failed to open log file:", err) } - defer logFile.Close() ... }
🧹 Nitpick comments (2)
cmd/root.go (2)
129-133: Minor note on default behavior.If
logLevelStris empty, it’s set to"info". This is fine, but confirm it aligns with user expectations when no level is specified.
135-139: Consider unifying the trace vs debug logging approach.You log setup details at
AtmosTraceLevelfor trace, butlog.Debugfor everything else. It might be simpler to adopt a consistent approach if you foresee more trace-level setups.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/root.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (2)
cmd/root.go (2)
12-12: Looks good adopting the Charmbracelet log import.No concerns here. The new import aligns well with your updated logging approach.
102-123: New logger initialization looks excellent.It's good to see the newly included
"Error"level handled. The approach for"Off"usingmath.MaxInt32effectively disables logging. Nicely done.
|
Please post an updated screenshot as evidence. |
| } | ||
|
|
||
| if strings.ToLower(atmosConfig.Logs.Level) == "trace" { | ||
| log.Log(logger.AtmosTraceLevel, "Set", "logs-level", logLevelStr, "logs-file", atmosConfig.Logs.File) |
There was a problem hiding this comment.
| log.Log(logger.AtmosTraceLevel, "Set", "logs-level", logLevelStr, "logs-file", atmosConfig.Logs.File) | |
| log.Trace("Set", "logs-level", logLevelStr, "logs-file", atmosConfig.Logs.File) |
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/root.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
cmd/root.go
[failure] 136-136:
undefined: log.Trace
🪛 GitHub Check: Build (ubuntu-latest, linux)
cmd/root.go
[failure] 136-136:
undefined: log.Trace
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
cmd/root.go (5)
12-12: Great choice on using charmbracelet/log for styled loggingThe switch to using the charmbracelet/log package aligns well with the PR objective of styling the Atmos logger. This package provides excellent support for structured, styled logging in terminal applications.
102-105: Good implementation of the styled loggerCreating a new AtmosLogger and setting it as the default logger is the right approach. This cleanly establishes the styled logger as the application's primary logging mechanism.
108-123: Comprehensive log level handlingThe switch statement now properly handles all log levels including the "Error" case that was previously requested. The addition of AtmosTraceLevel is also a good enhancement for more detailed logging when needed.
129-134: Good improvement to user feedbackUsing the exact log level string from the config provides more consistent output and better user experience. The default fallback to "info" is also a sensible approach.
82-100: Keep the output determination logic unchangedYou've correctly maintained the existing logic for determining the output writer based on configuration, which ensures backward compatibility.
This reverts commit 4820f56.
|
|
| @@ -1,3 +1,3 @@ | |||
| DEBU Set logs-level=debug logs-file=/dev/stderr | |||
| TRCE Set logs-level=trace logs-file=/dev/stderr | |||
There was a problem hiding this comment.
This is wierd to me. We shouldn't dynamically set the log level of the log output based on the configuration.
There was a problem hiding this comment.
Ok, never mind. I thought we were doing something dynamically, such that if I set Log level to Info, that this would then say:
INFO Set logs-level=info logs-file=/dev/stderr
Please confirm this is not the case.
| if strings.ToLower(atmosConfig.Logs.Level) == "trace" { | ||
| log.Log(logger.AtmosTraceLevel, "Set", "logs-level", logLevelStr, "logs-file", atmosConfig.Logs.File) | ||
| } else { | ||
| log.Debug("Set", "logs-level", logLevelStr, "logs-file", atmosConfig.Logs.File) |
There was a problem hiding this comment.
Here we use log.Debug, but in the logs we output TRCE
We should be consistent.
https://github.com/cloudposse/atmos/pull/1135/files#r2004173296
I'm okay changing it to Trace, but then we should have it be that consistently.
what
New Style for atmos logger with theme
why
references
Summary by CodeRabbit
New Features
Bug Fixes
Refactor