-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
etcdserver: add auth revision to AuthStatus to improve observability and testability #11659
Conversation
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.
Thanks for your change. Could you move the field and the code to fill the field to AuthStatus
?
Agree with @mitake, adding a field in AuthStatus API sounds good. In the short term, if you want something available in etcd v3.4, consider adding a metric (metric is usually backportable). |
7672ad4
to
606c695
Compare
606c695
to
bb36d9b
Compare
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! Looks good in general. Not exactly sure why v3election.swagger.json
and v3lock.swagger.json
were updated because there was no change to those APIs - I guess it is because of a different version of protoc-gen-swagger?
Thanks for response. |
@wswcfan I am not an expert on this. My suggestion is: 1. open a separate PR to update Any concerns on moving to latest |
Sorry, my mistake. the |
bb36d9b
to
8f9732e
Compare
This sounds good to me. If it causes any backward compatibility issue or other issue, we could always revert it. |
8f9732e
to
15eeb2c
Compare
Codecov Report
@@ Coverage Diff @@
## master #11659 +/- ##
==========================================
- Coverage 66.41% 66.2% -0.21%
==========================================
Files 402 402
Lines 36661 36667 +6
==========================================
- Hits 24349 24276 -73
- Misses 10826 10890 +64
- Partials 1486 1501 +15
Continue to review full report at Codecov.
|
I have split this PR into two commits, the first one just generate swagger doc, it may look cleaner now. |
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. Thanks!
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, thanks! I'm merging this.
This pr is a completion for last pr #11652 , to improve the auth revision's observability and testability