-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Auto-generate gogo annotations for api_v3 #6233
Conversation
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
665fd40
to
01dc254
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6233 +/- ##
==========================================
- Coverage 96.45% 96.45% -0.01%
==========================================
Files 355 355
Lines 20157 20151 -6
==========================================
- Hits 19442 19436 -6
Misses 528 528
Partials 187 187
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Signed-off-by: Yuri Shkuro <[email protected]>
## Which problem is this PR solving? - Part of jaegertracing/jaeger#4150 - Having gogo annotations in the IDL makes it difficult to users to build clients ## Description of the changes - Remove gogo annotations - Rename num_traces to search_depth ## How was this change tested? - Tested in jaegertracing/jaeger#6233 --------- Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Test this change in this draft PR: #6237 BTW, moving gogoproto related lines out is a great idea, since original proto file could be used by other programming languages . Also, could you merge this first? Then I would rebase my PR: #6159 |
@rim99 please approve this change and I will merge it. |
@yurishkuro woud you like to regenerate file Because to manage all test passed, I had to make this change: d6eeed8#diff-9f4e53eb6033f5e7d7af97a3fd7328e26bd3e5e948e01276cb85b3abfc0b2432 |
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.
NVM, I think that change might be related to my changes
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test