Added feature to switch to remote vcontrold #10
Conversation
…ally. Uses a config flag and host/port for connection with defaults to localhost:3002.
| @@ -18,9 +18,13 @@ else | |||
| export MQTT_PASSWORD=$(bashio::services mqtt "password") | |||
| export MQTT_TOPIC="openv" | |||
|
|
|||
| export VCONTROL_HOST=$(bashio::config 'vcontrol_host') | |||
There was a problem hiding this comment.
To avoid misunderstandings when remote_vcontrol is set to false, we should force the host/port to localhost. What do you think ?
There was a problem hiding this comment.
Yes, that's even better, but only if you can deactivate them in the config UI IMHO (like adding a ?)
I would avoid forcing values in the script without being able to deactivate them for the end user so one is not confused and inputs are not in conflict with the actual values that are used to run the service.
If this is not possible we can still rename the config flag and explain - e.g. using a flag "start local vcontrold". Will test it later.
There was a problem hiding this comment.
What about turning it around? If vcontrol_host/vcontrol_port is set, then it is assumed that it's a remote vcontrold.
If it's unset - the vcontrold is started and localhost:3002 is used.
There was a problem hiding this comment.
Yes, also a good idea. How about this: We remove the remote-switch, set defaults to null (hidden under advanced options) and assume local vcontrold on localhost:3002 in case there is no value set?
That way we stay backwards-compatible and have the same look-and-feel in the configs.
There was a problem hiding this comment.
Will push a test version later today/tonight once I have tested it locally.
There was a problem hiding this comment.
Hi @Schm1tz1
Thanks for the contribution!
Could you maybe also integrate a remote mqtt configuration? I tried my luck here:
https://github.com/ppuetsch/homeassistant-vcontrol/tree/main
However, I wasn't able to (sucessfully) test it as during my test still the main docker images (without the changes to the run scripts) were used. I right now don't have the time to dig deeper into that - but If you like the changes, maybe you can integrate them into your PR.
Thank you :-)
There was a problem hiding this comment.
Hi,
it's about the docker image pull policy. To test locally, simply comment out the image-line so it gets re-created upon installation:
https://github.com/Schm1tz1/homeassistant-vcontrol/blob/remote-vcontrold/vcontrold/config.yaml#L70
There was a problem hiding this comment.
Hi, it's about the docker image pull policy. To test locally, simply comment out the image-line so it gets re-created upon installation: https://github.com/Schm1tz1/homeassistant-vcontrol/blob/remote-vcontrold/vcontrold/config.yaml#L70
Thank you! 👍
There was a problem hiding this comment.
@Alexandre-io - took a look at the suggested changes and also the suggestions made by @ppuetsch. I think if we let the end-user switch by simply setting a different host/port or staying with the defaults we don't break anything, stay backwards-compatible and keep the configuration simple. To avoid restarts of the services due to non-existing vcontrold I needed to add a dummy tail-f service. Please have a look. It is already running in my basement :-)
…s suggested and only starting service in case we are using vcontrold@localhost, otherwise we are assuming a remote vcontrold
|
LGTM 👍 Clever solution, thanks guys! |
Added feature to switch to remote vcontrold instead of running it locally. Uses a config flag and host/port for connection with defaults to localhost:3002, see #9