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

Add Signal Messenger integration #28537

Merged
merged 19 commits into from Dec 8, 2019

Conversation

bbernhard
Copy link
Contributor

@bbernhard bbernhard commented Nov 4, 2019

Description:

Something that I would really like to see being supported by Home Assistant is the Signal Messenger. Unfortunately, there seems to be no official REST API (at least I haven't found any), that's why I started writing a small REST wrapper API around the excellent signal-cli command line tool.

The whole thing is working for almost 6 months without any problems (my Home Assistant instance sends signal messages on a daily basis) and I feel now confident enough to create a PR to get that whole thing into the Home Assistant release branch.

Unfortunately, I haven't found any info whether an external dependency (docker container) to an open source project is allowed or not?

Anyway, I've already created this PR request here. I tried to do my best to tick off all the items on the developer checklist, but can't rule out that I've missed something. So any code review is highly appreciated!

If a dependency to a docker container service is a no-go, please just decline this PR.

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#11100

Example entry for configuration.yaml:

notify:
  - name: signal
    platform: signalmessenger
    url: "http://127.0.0.1:8080" # the URL where the Signal Messenger REST API is listening 
    number: YOUR_PHONE_NUMBER # the sender number
    recipients: # one or more recipients
      - RECIPIENT1

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [The manifest file][manifest-docs] has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

@homeassistant
Copy link
Contributor

Hi @bbernhard,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@fabaff
Copy link
Member

fabaff commented Nov 4, 2019

Unfortunately, I haven't found any info whether an external dependency (docker container) to an open source project is allowed or not?

The issue with the Docker approach is that not all systems will support this, especially a Home Assistant installation. Dependency on a binary (speaking about signal-cli) on the other hand would be possible.

At first glances it could works as a Hass.io Add-on.

@bbernhard
Copy link
Contributor Author

The issue with the Docker approach is that not all systems will support this, especially a Home Assistant installation. Dependency on a binary (speaking about signal-cli) on the other hand would be possible.

The main reason why I went for a dockerized approach was, that didn't want to "poison" my Home Assistant docker container. Furthermore, I had the issue that I couldn't get signal-cli (a java app) to work with alpine based containers (I think the problem is musl...with glibc everything works flawlessly).

Not sure if it makes it any better, but the REST API wrapper around signal-cli can also be run standalone (it's a simple golang app). Personally I am more in favor of a REST API instead of a commandline interface (REST API doesn't necessarily need to run on the same host, it's versionised, has (usually) a nicer interface, etc.).

@MartinHjelmare MartinHjelmare changed the title Signal Messenger integration Add Signal Messenger integration Nov 6, 2019
@home-assistant home-assistant deleted a comment from homeassistant Nov 6, 2019
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Besides @fabaff 's comment on the docker dependency, the interface code needs to move to a pypi package.

homeassistant/components/signalmessenger/notify.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Nov 6, 2019
@bbernhard
Copy link
Contributor Author

Thanks for your suggestions - very much appreciated!
The PyPi package should be an easy one to add.

If I would add a PyPi package, is there any chance to get the PR merged into the release branch or is the docker stuff a block here? If so, what options do I have? Is a Hass.io Addon the only option, or is there something else I can do?

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Nov 7, 2019

I think it could be ok to rely on a docker container. We have precedence with eg deconz where it's the recommended way (besides the hass.io addon) to add deconz device support:
https://www.home-assistant.io/integrations/deconz/#recommended-way-of-running-deconz

What do you think @fabaff?

The hass.io addon can come later to create support for the wider audience.

@DeviousPenguin
Copy link

DeviousPenguin commented Nov 7, 2019

I also use signal-cli, it's a great little tool. I currently use it to send messages from HA to signal messenger. A proper signal integration would be awesome. 👍

I found using DBus a nice way of getting messages to signal-cli, it runs in the background as a service, I just use dbus-send from the command line currently, but there is a Python module to send DBus payloads.

The downside of this is that setting up signal-cli to use the DBus is a little more tricky, so a HassIO addon/docker image may be the way to go someone wants a '1 click install' without having to mess around with setting up a SystemD service. Also the API wrapper would be far better if you have signal-cli on a different machine to the one running HA, as Dbus is for the local system only.

@bbernhard
Copy link
Contributor Author

I think it could be ok to rely on a docker container.

The hass.io addon can come later to create support for the wider audience.

That would be awesome - would really appreciate that solution.

Please let me know as soon as you guys have made a decision. I'll then start with the PyPi library for the service specific code.

I also use signal-cli, it's a great little tool.

totally agreed! Never had any issues with it, it's really well maintained!

Also the API wrapper would be far better if you have signal-cli on a different machine to the one running HA, as Dbus is for the local system only.

totally agreed! :)

@fabaff
Copy link
Member

fabaff commented Nov 9, 2019

From a user point of view there is not much difference between installing a binary or running a container. Thus, both are requirements and the users have to do some additional work. I see no problem with that and I just wanted to mention it because there is a much bigger user base here than deCONZ (I assume that).

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

* re-ordered import statements
* removed empty "requirements" list (not needed)
@springstan
Copy link
Member

@bbernhard the error in FullCheck Pylint is unrelated to this PR. The tests need to be rerun manually or you can force a rerun by closing and reopening this PR.

@bbernhard bbernhard closed this Nov 29, 2019
Dev automation moved this from Reviewer approved to Cancelled Nov 29, 2019
@bbernhard bbernhard reopened this Nov 29, 2019
Dev automation moved this from Cancelled to Incoming Nov 29, 2019
Dev automation moved this from Incoming to Review in progress Nov 29, 2019
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Good!

Dev automation moved this from Review in progress to Reviewer approved Dec 1, 2019
@MartinHjelmare
Copy link
Member

Should we rename the integration to signal_messenger? We normally separate words with underscore in integration names.

@bbernhard
Copy link
Contributor Author

Should we rename the integration to signal_messenger? We normally separate words with underscore in integration names.

For me both is fine. I could also rename it to signal if you prefer a shorter name (then it would be similar to the telegram messenger integration https://www.home-assistant.io/integrations/telegram/).

Just let me know which one you prefer :)

@MartinHjelmare
Copy link
Member

Only signal is very generic. There's no brand name here, right? We normally try to use brand names. I think signal_messenger is ok.

@bbernhard
Copy link
Contributor Author

Hi,

just a quick check (as this is my first PR in the HA repository): Is there anything that needs to be done from my side?

.coveragerc Outdated Show resolved Hide resolved
Dev automation moved this from Reviewer approved to Review in progress Dec 8, 2019
Dev automation moved this from Review in progress to Reviewer approved Dec 8, 2019
@MartinHjelmare MartinHjelmare merged commit d451e54 into home-assistant:dev Dec 8, 2019
Dev automation moved this from Reviewer approved to Done Dec 8, 2019
@lock lock bot locked and limited conversation to collaborators Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants