Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

Disable sdv.databroker.v1 by default #739

Closed

Conversation

argerus
Copy link
Contributor

@argerus argerus commented Feb 15, 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.

- 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.
@erikbosch
Copy link
Contributor

Great progress! When everything is working we need to update the documentation. I noticed some inconsistencies in the existing documentation. In the main README we mention the --protocol option, but we do not mention it in the user guide.

Do we possibly need some form of changelog to keep track of more important changes (like this one?). I guess we also need to discuss if this change is so big that it implies that we shall call the next Databroker version 0.5?

@erikbosch
Copy link
Contributor

I did some sanity checks with KUKSA Python SDK and databroker-cli. No problems observed.

A possible improvement in databroker-cli could be to give a more intuitive error message if trying to use sdv.databroker.v1 but it is not supported by Databroker. Now it only reports "unimplemented", which is correct, but how much work would it be to give a warning "If you want to use sdv.databroker.v1 you must start databroker with --enable-databroker-v1 argument".

But it is not a deal breaker for me accepting the change, so if documentation is updated I am happy to approve.

erik@debian4:~/kuksa.val/kuksa_databroker$ cargo run --bin databroker-cli -- -p sdv.databroker.v1
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `/home/erik/kuksa.val/target/debug/databroker-cli -p sdv.databroker.v1`
Using sdv.databroker.v1

  ⠀⠀⠀⢀⣤⣶⣾⣿⢸⣿⣿⣷⣶⣤⡀
  ⠀⠀⣴⣿⡿⠋⣿⣿⠀⠀⠀⠈⠙⢿⣿⣦⠀
  ⠀⣾⣿⠋⠀⠀⣿⣿⠀⠀⣶⣿⠀⠀⠙⣿⣷   
  ⣸⣿⠇⠀⠀⠀⣿⣿⠠⣾⡿⠃⠀⠀⠀⠸⣿⣇⠀⠀⣶⠀⣠⡶⠂⠀⣶⠀⠀⢰⡆⠀⢰⡆⢀⣴⠖⠀⢠⡶⠶⠶⡦⠀⠀⠀⣰⣶⡀
  ⣿⣿⠀⠀⠀⠀⠿⢿⣷⣦⡀⠀⠀⠀⠀⠀⣿⣿⠀⠀⣿⢾⣏⠀⠀⠀⣿⠀⠀⢸⡇⠀⢸⡷⣿⡁⠀⠀⠘⠷⠶⠶⣦⠀⠀⢠⡟⠘⣷
  ⢹⣿⡆⠀⠀⠀⣿⣶⠈⢻⣿⡆⠀⠀⠀⢰⣿⡏⠀⠀⠿⠀⠙⠷⠄⠀⠙⠷⠶⠟⠁⠀⠸⠇⠈⠻⠦⠀⠐⠷⠶⠶⠟⠀⠠⠿⠁⠀⠹⠧
  ⠀⢿⣿⣄⠀⠀⣿⣿⠀⠀⠿⣿⠀⠀⣠⣿⡿
  ⠀⠀⠻⣿⣷⡄⣿⣿⠀⠀⠀⢀⣠⣾⣿⠟    databroker-cli                
  ⠀⠀⠀⠈⠛⠇⢿⣿⣿⣿⣿⡿⠿⠛⠁     v0.4.1                        

Successfully connected to http://127.0.0.1:55555/
[metadata]  Unimplemented  
sdv.databroker.v1 > get Vehicle.Speed
[get]  Unimplemented  
sdv.databroker.v1 > 

@SebastianSchildt
Copy link
Contributor

Integration test currently fails as this https://github.com/eclipse/kuksa.val/blob/master/kuksa_databroker/integration_test/test_databroker.py depends on the old API.

Should be updated. This could be rewritten using the python sdk, but as this is a very basic "GRPC-level" test might be better to keep it like this, just chosing a number of kuksa.val.v1 calls. This is currently not a real test whether the API behaves 100% in a certain way, but rather a rain check that it came up at all, is connectable and seems to to do "something useful"

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

@boschglobal boschglobal closed this by deleting the head repository Mar 21, 2024
@erikbosch
Copy link
Contributor

Reopening all PRs closed by accident by boschglobal maintenance

@erikbosch erikbosch reopened this Mar 21, 2024
@SebastianSchildt
Copy link
Contributor

is there still progress here? Somebody fixing the test?

@erikbosch
Copy link
Contributor

is there still progress here? Somebody fixing the test?

We have sort of agreed to NOT fix this in this repo to avoid causing backward compatibility for anyone using the old repo, but rather fix it after migration, i.e. in https://github.com/eclipse-kuksa/kuksa-databroker

@argerus argerus closed this Mar 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants