-
Notifications
You must be signed in to change notification settings - Fork 18
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
Updating documentation and deprecating old APIs #105
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 59.23% 59.33% +0.10%
==========================================
Files 33 33
Lines 16063 16067 +4
==========================================
+ Hits 9515 9534 +19
+ Misses 6548 6533 -15 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
doc/wildcard_matching.md
Outdated
### Matching rules | ||
# Wildcard Matching rules | ||
|
||
*Note! This document applies to `sdv.databroker.v1` and `kuksa.val.v1` only!* |
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.
It also applies to list_metadata
in kuksa.val.v2, see https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/databroker/src/grpc/kuksa_val_v2/val.rs#L502
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.
That seems to be true. But we do not care about filter
at all right? I thought we used a different approach there as the names root
and filter
seems to imply something else.
As it is implemented today we should better just have a single field and call it path
, or?
In the description we say that root
shall be a root branch, but in all test cases we using paths to a signal, like "test.datapoint1"
.
// List metadata of signals matching the request.
//
// Returns (GRPC error code):
// NOT_FOUND if the specified root branch does not exist.
// UNAUTHENTICATED if no credentials provided or credentials has expired
// INVALID_ARGUMENT if the provided path or wildcard is wrong.
So NOT_FOUND
actually is returned if there are no matching entities. And most of the examples in https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/doc/wildcard_matching.md has nothing to with a root branch.
So what shall we do - adapt Proto definition to implementation?
FYI @lukasmittag @argerus
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.
See #106
We need to align what to do short-term (for next release) as well as long term for ListMetadata
.
Doing as little as possible is one option - i.e. just adding a comment "Please do not use" in the *.proto files and documentation.
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.
I tried to add some text both explaining how it works today but also a remark that it may change
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.
Looks good to me.
doc/protocol.md
Outdated
| ------------------------ |-----|-----|-----|-----|-----|-----|-----|-----|-----| | ||
| gRPC (kuksa.val.v2) | Yes | Yes | Yes | No | No | No | Yes | No | Yes | ||
| gRPC (kuksa.val.v1) *Deprecated!* | Yes | Yes | Yes | Yes | Yes | Yes | No | No | No | ||
| gRPC (sdv.databroker.v1) *Deprecated!* | Yes | Yes | Yes | Yes | Yes | Yes | No | No | No |
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.
sdv.databroker.v1
does not support getting or subscribing to a "target value". And (hence) when it comes to supporting actuation of VSS signals, it's behaviour is more in line with actuate
and kuksa.val.v2
.
This PR covers the following: