-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
New layout for proto definitions and generated files #1427
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.
Should there be a corresponding Makefile change included here?
Also, you mentioned doing something special to keep the swagger file together.
}; | ||
} | ||
} | ||
|
||
enum SamplingStrategyType { |
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.
Want to be careful with this one and test whether the older agent using grpc to pull sampling strategies would still work once the collector is upgraded to these changes. Otherwise we don't give a clean upgrade path to people.
I assume since we're not changing the actual proto namespace, it should work fine, but best to verify.
It seems you're using different versions of protoc or the plugins. When I run
|
|
d5a79fb
to
9ceffa3
Compare
Signed-off-by: Annanay <annanay.a@media.net>
Signed-off-by: Annanay <annanay.a@media.net>
Signed-off-by: Annanay <annanay.a@media.net>
Signed-off-by: Annanay <annanay.a@media.net>
Signed-off-by: Annanay <annanay.a@media.net>
I'm not sure why the |
You need to run "make proto-install" before generating, to ensure the right versions of plugins. |
Signed-off-by: Annanay <annanay.a@media.net>
6abd185
to
353fdc7
Compare
Signed-off-by: Annanay <annanay.a@media.net>
Signed-off-by: Annanay <annanay.a@media.net>
Codecov Report
@@ Coverage Diff @@
## master #1427 +/- ##
======================================
Coverage 100% 100%
======================================
Files 165 165
Lines 7510 7510
======================================
Hits 7510 7510 Continue to review full report at Codecov.
|
Signed-off-by: Annanay <annanay.a@media.net>
@@ -1,7 +1,7 @@ | |||
{ | |||
"swagger": "2.0", | |||
"info": { | |||
"title": "api_v2.proto", | |||
"title": "api_v2/collector.proto", |
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.
This is a little confusing. The generated swagger file has all definitions from sampling.proto
as well, but not sure why lists title as api_v2/collector.proto
good stuff! |
Thanks! 😄 PS: Does this fix #904? |
Thanks for the ping, yes I believe it does. |
Which problem is this PR solving?
Short description of the changes
api_v2.proto
into respective services -collector
andsampling
(manager)swagger.json
from the multiple proto definitions