fix log level priorty #1031
fix log level priorty #1031
Conversation
a47bb51 to
568597b
Compare
tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stdout.golden
Show resolved
Hide resolved
...stCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stdout.golden
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThis pull request refines the logging configuration in Atmos by adjusting how command-line flags are handled. The changes conditionally update the configuration only when flags are explicitly modified, and a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant RootCmd
participant parseFlags
participant Env
participant Config
CLI->>RootCmd: Start command execution
RootCmd->>parseFlags: Parse logging-related flags
parseFlags-->>RootCmd: Return parsed flag values
RootCmd->>Env: Check environment variables (ATMOS_LOGS_LEVEL, ATMOS_LOGS_FILE)
RootCmd->>Config: Load configuration file settings
RootCmd-->>CLI: Initialize logging (using flags > ENV > config)
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/test-cases/log-level-validation.yaml (5)
20-39: Robust Check for Invalid Log Level in Environment VariableThis test correctly ensures that an invalid log level provided through the environment variable results in an error. For consistency with other tests (see later cases where strings are quoted), consider quoting the value on line 33 (e.g.,
"XTrace") to avoid any unexpected YAML parsing behavior.
92-109: Ensure Env Variable Overrides Config for Log LevelThis test demonstrates that the log level provided via the environment variable (set to "Debug") takes priority over the configuration setting, as shown by the expected debug log output. Note: the test name uses "priortized"—a quick typo fix to "prioritized" would improve clarity.
110-128: Flag Supersedes Env and Config for Log LevelThis test effectively checks that the command-line flag for log level overrides both the environment variable and configuration settings. As with the previous test, consider correcting "priortized" to "prioritized" in the test name.
129-147: Env Variable Correctly Prioritizes Log File LocationThe test verifies that when the log file is specified through the environment variable (
ATMOS_LOGS_FILEset to/dev/stdout), it directs the debug logs as expected. A minor note: please update "priortized" to "prioritized" in the test name for consistency.
148-168: Flag Overrides Env for Log File DestinationThis test case confirms that the command-line flag for the log file takes precedence over the environment variable (which is set to
/dev/stderr), ensuring the logs are directed to/dev/stdoutas intended. A small update to fix "priortized" to "prioritized" in the test name is recommended for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/root.go(3 hunks)tests/fixtures/scenarios/priority-log-check/atmos.yaml(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stderr.golden(0 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden(0 hunks)tests/snapshots/TestCLICommands_Valid_log_file_in_env_should_be_priortized_over_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stdout.golden(1 hunks)tests/test-cases/log-level-validation.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stderr.golden
✅ Files skipped from review due to trivial changes (5)
- tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stdout.golden
- tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stdout.golden
- tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stderr.golden
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stdout.golden
- tests/snapshots/TestCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (9)
cmd/root.go (2)
48-57: Verify if conditional flag handling is necessary.The conditional setting of log level and file only when flags are changed is a good practice. However, let's verify if this change is required since there's a TODO comment expressing uncertainty.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if there are any instances where the flag values are used without checking if they were changed rg -A 5 "cmd\.Flags\(\)\.GetString\(\"logs-(level|file)\"\)" --glob '!{cmd/root.go}'Length of output: 520
Conditional flag handling appears both intentional and necessary.
Our investigation shows that outside of
cmd/root.go, flags like"logs-level"are used directly (as seen incmd/validate_editorconfig.go), whereas incmd/root.gothe conditional assignment ensures thatConfigAndStacksInfoonly captures user-specified values. This prevents inadvertently overriding defaults when users don’t modify these flags. Therefore, the conditional checks are appropriate and align with the rest of the codebase.
127-161: Consider using Viper for flag management.While the manual flag parsing implementation is thorough, using Viper would be a more robust solution as mentioned in the PR objectives. Viper provides built-in support for managing configuration priorities between flags, environment variables, and config files.
✅ Verification successful
🌐 Web query:
What are the benefits of using Viper over manual flag parsing in Go?Length of output: 2902
Review Comment – Consider using Viper for Flag Management
The manual flag parsing implementation in
cmd/root.gois solid and handles common flag patterns well. However, as the web query confirms, using Viper offers several benefits:
- Unified Configuration Management: Viper integrates multiple configuration sources (flags, environment variables, config files, etc.) with a clear, hierarchical precedence.
- Reduced Boilerplate and Automatic Binding: Viper seamlessly binds Cobra flags and automatically manages type conversion, reducing the manual overhead.
- Advanced Features: Includes support for live configuration reloading, structured data parsing, and unified access via
viper.Get*()methods.Given these advantages and the PR objectives, it would be worthwhile to consider refactoring the manual flag parsing in favor of Viper.
tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stderr.golden (1)
1-4: LGTM! Test snapshot validates priority order.The debug logs correctly demonstrate that command-line arguments take precedence over environment variables, validating the priority implementation.
tests/snapshots/TestCLICommands_Valid_log_file_in_env_should_be_priortized_over_config.stdout.golden (1)
1-7: LGTM! Test snapshot validates environment variable priority.The debug logs correctly demonstrate that environment variables take precedence over configuration file settings.
tests/fixtures/scenarios/priority-log-check/atmos.yaml (1)
1-9: LGTM! Configuration provides sensible defaults.The configuration file is well-structured with appropriate default values for logging level and output destination.
tests/test-cases/log-level-validation.yaml (4)
2-19: Clear Validation for Invalid Log Level in Config FileThis test case cleanly verifies that an invalid log level specified in the configuration file leads to a failure (exit code 1) with an appropriate error message.
40-54: Solid Test for Valid Log Level in Config FileThe test confirms that when a valid log level is set in the configuration file, the command executes successfully and produces output matching the expected regex pattern. Looks good!
55-73: Effective Validation of Valid Log Level via Environment VariableThis test efficiently verifies that setting a valid log level using the environment variable is correctly picked up, as indicated by the debug log message in stderr.
74-91: Correct Prioritization for Valid Log Level via Command Line FlagThe test case neatly confirms that specifying the log level via the command line flag works as expected, taking precedence over other configurations with an empty stderr.
|
These changes were released in v1.160.2. |
* fix log level priorty * fix unwanted flag being set * add tests for log level priority * added todo comment with more clearity * Add tests for logs-file * update comment
Known Issues
what
why
references
Summary by CodeRabbit
New Features
Tests