New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add flag for debug logs #1166
Add flag for debug logs #1166
Conversation
operators/cmd/main.go
Outdated
|
||
cobra.OnInitialize(func() { | ||
logf.SetLogger(logf.ZapLogger(dev.Enabled)) | ||
level, _ := rootCmd.Flags().GetString(logLevelFlag) | ||
logLevel := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would tend to name the logLevel
boolean variable enableDebugLevel
instead of logLevel
to better reflect what it does and then name level
in logLevel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that but wasn't sure if we might want to have a separate debug mode (maybe enabling profiling if we're not already?). That's not a very persuasive argument though so I'm good with changing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds a distinct log level flag to enable debug logs (rather than as part of the development flag), as part of #305
I wasn't sure if we wanted a flag or env var, so I followed how it was previously set up. Definitely open to changing it. Modifying env vars (or a env vars via a config map) feels slightly preferable but I don't feel strongly either way.