Skip to content
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

Add reflection support for kuksa.val.v2 #89

Conversation

rafaeling
Copy link
Contributor

@rafaeling rafaeling commented Oct 29, 2024

Release binaries generated locally went from 6.3 MB to 6.5 MB after reflection support was added for kuksa.val.v2

It helps to test kuksa.val.v2 with grpcurl until databroker-cli is updated, i.e:

grpcurl -d '{ "signal_id": { "path": "Vehicle.Speed" }, "data_point": { "timestamp": "2024-10-29T12:04:31.463835877Z", "value": { "float": 75.5 } }}' -plaintext localhost:55555 kuksa.val.v2.VAL.PublishValue

grpcurl -d '{ "signal_id": { "path": "Vehicle.Speed" } }' -plaintext localhost:55555 kuksa.val.v2.VAL.GetValue

grpcurl -d '{ "signal_paths": ["Vehicle.Speed"]}' -plaintext localhost:55555 kuksa.val.v2.VAL.Subscribe

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 58.92%. Comparing base (62552a5) to head (8483c51).
Report is 4 commits behind head on feature/databroker-api-v2.

Files with missing lines Patch % Lines
databroker/src/grpc/server.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@                      Coverage Diff                      @@
##           feature/databroker-api-v2      #89      +/-   ##
=============================================================
- Coverage                      58.94%   58.92%   -0.02%     
=============================================================
  Files                             33       33              
  Lines                          15867    15872       +5     
=============================================================
  Hits                            9353     9353              
- Misses                          6514     6519       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lukasmittag
Copy link
Contributor

If we enable it shouldn't we enable it for the other interfaces as well not only v2?

@erikbosch
Copy link
Contributor

Should we possibly add some info on how this can be used, either in the wiki (https://github.com/eclipse-kuksa/kuksa-databroker/wiki/Release-Testing ? ) or somewhere in https://github.com/eclipse-kuksa/kuksa-databroker/tree/main/doc

@rafaeling
Copy link
Contributor Author

Should we possibly add some info on how this can be used, either in the wiki (https://github.com/eclipse-kuksa/kuksa-databroker/wiki/Release-Testing ? ) or somewhere in https://github.com/eclipse-kuksa/kuksa-databroker/tree/main/doc

I think it would be good to add it to the Release-Testing to ensure all errors are returned correctly.

Copy link
Contributor

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. For me it is OK that we only have discovery for the new API. I tested with the command below and it reported what I expected.

erik@debian6:~/kuksa.val$ docker run --network=host fullstorydev/grpcurl  --plaintext -vv 127.0.0.1:55555 describe
kuksa.val.v2.VAL is a service:
service VAL {

But my approval is conditioned on that we add a short section in the release testing wiki, possibly just stating that you should run a command like above. Even better would be if we could have an automatic test, but that would require more work I think

@rafaeling
Copy link
Contributor Author

Looks good to me. For me it is OK that we only have discovery for the new API. I tested with the command below and it reported what I expected.

erik@debian6:~/kuksa.val$ docker run --network=host fullstorydev/grpcurl  --plaintext -vv 127.0.0.1:55555 describe
kuksa.val.v2.VAL is a service:
service VAL {

But my approval is conditioned on that we add a short section in the release testing wiki, possibly just stating that you should run a command like above. Even better would be if we could have an automatic test, but that would require more work I think

@erikbosch there it is https://github.com/eclipse-kuksa/kuksa-databroker/wiki/Release-Testing#testing-databroker-kuksavalv2-api-methods-with-grpcurl

@rafaeling rafaeling merged commit aa4444f into eclipse-kuksa:feature/databroker-api-v2 Nov 5, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants