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

Disable sdv.databroker.v1 by default #20

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

argerus
Copy link
Contributor

@argerus argerus commented Apr 24, 2024

  • Disable sdv.databroker.v1 by default. It can be enabled by supplying a flag at startup.
  • Change databroker-cli to use kuksa.val.v1 by default.

@argerus argerus requested a review from lukasmittag April 24, 2024 11:37
Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

Some minor naming discussions

(
"subscribe",
"<QUERY>",
"Subscribe to signals with QUERY, if you use kuksa feature comma separated list",
),
("feed", "<PATH> <VALUE>", "Publish signal value"),
("publish", "<PATH> <VALUE>", "Publish signal value"),
Copy link
Contributor

Choose a reason for hiding this comment

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

provide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that crossed my mind as well. I'm not against it, but my idea is that publish is that act of publishing a single signal value, while "provide" might be better suited for describing the act of continuously providing a signal. "publish" would then be the logical counterpart to "subscribe".

I suppose we will have a discussion around this in the API design rounds. I just couldn't stand having feed still there 🥲

@@ -42,7 +42,7 @@ const CLI_COMMANDS: &[(&str, &str, &str)] = &[
"<QUERY>",
"Subscribe to signals with QUERY, if you use kuksa feature comma separated list",
),
("feed", "<PATH> <VALUE>", "Publish signal value"),
("publish", "<PATH> <VALUE>", "Publish signal value"),
Copy link
Contributor

Choose a reason for hiding this comment

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

provide?

.add_service(
let kuksa_val_v1 = {
if apis.contains(&Api::KuksaValV1) {
Some(kuksa::val::v1::val_server::ValServer::with_interceptor(
Copy link
Contributor

Choose a reason for hiding this comment

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

val_server can maybe cause confusion with old kuksa_val_server in c++?

Copy link
Contributor Author

@argerus argerus Apr 25, 2024

Choose a reason for hiding this comment

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

This was not changed as part of this commit. It is referencing generated code, which having a protobuf service definition like this:

service VAL {
  ...
}

will generate code <snake case service name>_server::<camel case service name>Server, i.e. val_server::ValServer.

- Disable sdv.databroker.v1 by default.
  It can be enabled by supplying a flag at startup.
- Change databroker-cli to use kuksa.val.v1 by default.
@argerus argerus force-pushed the feature/disable_sdv branch from 35035df to 9f7ec57 Compare April 25, 2024 08:31
Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

See the comments above. I agree with them an we can merge this.

@argerus argerus merged commit 700ff8e into eclipse-kuksa:main Apr 25, 2024
16 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.

2 participants