generated from home-assistant/addons-example
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Added feature to switch to remote vcontrold #10
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
To avoid misunderstandings when
remote_vcontrol
is set tofalse
, we should force the host/port to localhost. What do you think ?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will push a test version later today/tonight once I have tested it locally.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 👍
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.
@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 :-)