fix(logger): respect log level from environment variable in dev mode #4083
Conversation
WalkthroughThis change updates the log level determination within the Changes
Sequence Diagram(s)sequenceDiagram
participant App as CreateApp Function
participant Env as Environment Variable ("loglevel")
participant Options as AppOptions
participant LogFlag as LogLevelFlag
App->>Options: Retrieve default log level
App->>Env: Check "loglevel" environment variable
alt Environment variable is provided and flag is explicitly set
App->>LogFlag: Set log level to env var value
else
App->>LogFlag: Set log level to default value
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
v2/internal/app/app_dev.go (1)
97-105:⚠️ Potential issueFix inconsistency in log level comparison.
The comparison at line 97 uses
appoptions.LogLevel.String()instead ofappLogLevel, which could lead to unexpected behavior when the environment variable is set. This means the environment variable value might be ignored when checking if the flag was explicitly set.Apply this fix:
-if loglevelFlag != nil && devFlags.Lookup("loglevel").Value.String() != appoptions.LogLevel.String() { +if loglevelFlag != nil && devFlags.Lookup("loglevel").Value.String() != appLogLevel {
🧹 Nitpick comments (1)
v2/internal/app/app_dev.go (1)
77-82: LGTM! Consider adding validation for environment variable value.The changes improve the log level configuration by respecting the environment variable. However, consider validating the environment variable value before using it.
Add validation for the environment variable value:
appLogLevel := appoptions.LogLevel.String() if loglevel != "" { + if loggerLevel, err := pkglogger.StringToLogLevel(loglevel); err == nil { + appLogLevel = loglevel + } else { + myLogger.Warning("Invalid log level in environment variable: %s", loglevel) + } - appLogLevel = loglevel }
|
Thanks 🙏 Please could you add an entry to the changelog located at |
Sure no problem, do you also want me to rebase the branch or do you prefer handling it? |
#4082
Summary by CodeRabbit
Refactor
Bug Fixes