-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
clientv3: Fix parsing of ETCD_CLIENT_DEBUG #14203
Conversation
Codecov Report
@@ Coverage Diff @@
## main #14203 +/- ##
==========================================
- Coverage 75.49% 75.25% -0.24%
==========================================
Files 455 455
Lines 36859 36859
==========================================
- Hits 27828 27740 -88
- Misses 7299 7375 +76
- Partials 1732 1744 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -51,7 +51,7 @@ func etcdClientDebugLevel() zapcore.Level { | |||
return zapcore.InfoLevel | |||
} | |||
var l zapcore.Level | |||
if err := l.Set(envLevel); err == nil { | |||
if err := l.Set(envLevel); err != nil { |
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.
The change should be good. But the message below is a little confusing (see line 55). Is ETCD_CLIENT_DEBUG
really deprecated? cc @serathius
Deprecated env ETCD_CLIENT_DEBUG value. Using default level: 'info'
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 parsed that sentence as the value being deprecated, rather than the setting.
#12786 introduced this code and explicitly brought back ETCD_CLIENT_DEBUG.
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.
Doesn't the following message make more sense? @ptabor
Invalid value for environment variable 'ETCD_CLIENT_DEBUG'. Using default level: 'info'
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.
Yes. That would be definitely better.
Thanks @Jille for the PR, please sign off the commit,
|
It checked `err == nil` rather than `err != nil`. Signed-off-by: Jille Timmermans <[email protected]>
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.
Thank you.
Please fix the error message as well (as proposed by @ahrtr).
Signed-off-by: Jille Timmermans <[email protected]>
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.
@Jille Could you backport this PR to 3.5? cc @serathius |
Sure: #14222 |
It checked
err == nil
rather thanerr != nil
.@ptabor
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.