-
Notifications
You must be signed in to change notification settings - Fork 74
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
Update proto for marshal issue #108
base: main
Are you sure you want to change the base?
Update proto for marshal issue #108
Conversation
Signed-off-by: rim99 <[email protected]>
7ba0fde
to
5f4ef40
Compare
What does "works normally" mean? What wasn't working? |
@yurishkuro Before this change, this test would fail It seems, server side cannot receive time values in request, and initialized these as 0001-01-01T00:00:00Z |
I don't follow. The test is passing in the main repo, against the official proto. What specifically makes it fail? |
The test in main repo doesn't test these 2 time fields. https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/apiv3/grpc_handler_test.go#L92-L94 In this proto file, tested time fields are here: https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v3/query_service.proto#L60 I think we can either add more gogoproto extensions like current way, or remove these 2 existing gogoproto extension lines
https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v3/query_service.proto#L33-L37 |
@yurishkuro Removed gogoproto lines in proto file. Kind feel this should be the correct direction to move, since gogoproto project is already deprecated. And also to keep align with rest of lines. |
Signed-off-by: rim99 <[email protected]>
c06731f
to
08204be
Compare
Which problem is this PR solving?
Resolves #4150
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test