Validate Atmos Log Levels #930
Conversation
Cerebrovinny
force-pushed
the
DEV-2693
branch
from
e3feb92 to
88592f5
Compare
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Cerebrovinny
force-pushed
the
DEV-2693
branch
from
7fc4fc8 to
88592f5
Compare
|
@Cerebrovinny also ensure this behavior is working with the |
|
Add test cases for each. It's sufficient to use tests/test-cases |
Cerebrovinny
force-pushed
the
DEV-2693
branch
from
88592f5 to
0007c6d
Compare
📝 WalkthroughWalkthroughThe pull request introduces comprehensive log level validation across the Atmos configuration system. Changes span multiple files in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/fixtures/valid-log-level/atmos.yaml (1)
1-8: Fix YAML formatting issues.The configuration is valid, but there are two formatting issues to address:
- Remove trailing spaces after
"**/*"on line 8- Add a newline at the end of the file
Apply this diff to fix the formatting:
logs: level: Trace file: /dev/stdout stacks: base_path: stacks included_paths: - - "**/*" + - "**/*" +🧰 Tools
🪛 yamllint (1.35.1)
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
[error] 8-8: trailing spaces
(trailing-spaces)
tests/fixtures/invalid-log-level/atmos.yaml (1)
8-8: Quick formatting fixes needed, warrior!Let's maintain clean YAML formatting:
- Add a newline at the end of file
- Remove trailing spaces after "**/*"
- - "**/*" + - "**/*" +🧰 Tools
🪛 yamllint (1.35.1)
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
[error] 8-8: trailing spaces
(trailing-spaces)
tests/test-cases/log-level-validation.yaml (2)
36-80: Strong positive test coverage, but let's fortify it further! 🏰The tests cover all input methods (config, env var, CLI) with valid log levels. However, based on the PR objectives, we should add:
- Case sensitivity tests (e.g., "DEBUG" vs "Debug")
- Empty log level validation
- Hierarchical logging tests (verify Debug shows Trace messages)
Would you like me to help generate additional test cases for these scenarios?
🧰 Tools
🪛 yamllint (1.35.1)
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
[error] 80-80: trailing spaces
(trailing-spaces)
80-80: Quick formatting fix needed, warrior!Add a newline at the end of file and remove trailing spaces.
- exit_code: 0 + exit_code: 0 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
[error] 80-80: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/config/utils.go(4 hunks)pkg/logger/logger.go(4 hunks)pkg/utils/log_utils.go(1 hunks)tests/fixtures/invalid-log-level/atmos.yaml(1 hunks)tests/fixtures/valid-log-level/atmos.yaml(1 hunks)tests/test-cases/log-level-validation.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/fixtures/invalid-log-level/atmos.yaml
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
[error] 8-8: trailing spaces
(trailing-spaces)
tests/fixtures/valid-log-level/atmos.yaml
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
[error] 8-8: trailing spaces
(trailing-spaces)
tests/test-cases/log-level-validation.yaml
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
[error] 80-80: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/logger/logger.go (4)
23-30: Well-structured log level hierarchy!The ordering from most verbose (Trace) to least verbose (Off) is logically organized and follows standard logging practices.
57-64: Improved log level validation with helpful error messages!The validation using a slice of valid levels is more maintainable, and the error message now includes valid options for better user experience.
Line range hint
112-120: Good catch on respecting LogLevelOff!The additional check ensures errors are not logged when the log level is set to Off, maintaining consistent behavior with the logging configuration.
123-129: Excellent implementation of hierarchical logging!The
isLevelEnabledmethod elegantly implements the hierarchical log level functionality, ensuring consistent behavior across all logging methods. The implementation correctly uses the level ordering to determine if a message should be logged.Also applies to: 132-152
pkg/utils/log_utils.go (1)
51-51: Verify log output formatting after newline removal.The removal of the newline character might affect log readability. Please verify that the log output remains properly formatted across different logging scenarios.
✅ Verification successful
Log formatting maintains proper readability ✓
The use of
Fprintlnautomatically adds newlines, maintaining consistent log formatting with other error logging implementations in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check log output formatting with different error messages # Test single-line error echo "Testing single-line error formatting..." rg -A 1 'logColor.Fprintln.*Error' . # Test multi-line error echo "Testing multi-line error formatting..." rg -A 1 'Error logging the error:' .Length of output: 525
pkg/config/utils.go (1)
362-366: Robust log level validation across all configuration sources!The implementation ensures that log levels are validated consistently whether they come from environment variables, command-line arguments, or configuration files. The validation-before-assignment pattern prevents invalid log levels from being set.
Also applies to: 405-409, 488-491
tests/fixtures/invalid-log-level/atmos.yaml (1)
1-4: Strong test fixture for negative testing! 💪The invalid log level "XTrace" is perfect for validating the error handling. This will help ensure our logging system remains robust.
tests/test-cases/log-level-validation.yaml (1)
1-35: Excellent negative test cases, commander! 🛡️The invalid log level tests are well-structured and verify proper error messages. The error message regex
\[Trace Debug Info Warning Off\]clearly communicates valid options to users.
| LogLevelDebug: 1, | ||
| LogLevelInfo: 2, | ||
| LogLevelWarning: 3, | ||
| LogLevelOff: 4, |
There was a problem hiding this comment.
This is interesting. Do we expose this option as well? Let's expose it and update docs.
what
why
We should not accept invalid log level to be set
Evidence:
BEFORE:
AFTER:
references
Summary by CodeRabbit
New Features
Bug Fixes
Tests