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

feat: support signal rest api notifications #650

Merged
merged 1 commit into from
Oct 8, 2022

Conversation

MrRagga-
Copy link
Contributor

Hi,
I created a notification for https://github.com/bbernhard/signal-cli-rest-api
If you are interested in this notification I can finish the missing things like documentation, enhance code, etc...?

@MrRagga-
Copy link
Contributor Author

Any objections?

@crazy-max
Copy link
Owner

If I understand correctly, this notification will be sent to the https://github.com/bbernhard/signal-cli-rest-api service and then be sent to Signal itself? If so I would be prefer to not use this intermediate service and directly send the notification to Signal.

Also looking at signal-cli-rest-api it looks like you can directly use the webhook notifier.

@MrRagga-
Copy link
Contributor Author

MrRagga- commented Jul 24, 2022

This rest API is just a small wrapper around https://github.com/AsamK/signal-cli which is a binary to talk with the Signal API, which has all the stuff implemented.
I already tried the webhook. But it currently does not support POST requests and not custom payload data.
Thus I can't use it right know.
On the other hand, isn't everything a webhook? I thought to have a more precise plugin might make more sense. But it is your project. I am also willing to modify the webhook to fit needs. Up to you?

@crazy-max
Copy link
Owner

This rest API is just a small wrapper around https://github.com/AsamK/signal-cli which is a binary to talk with the Signal API, which has all the stuff implemented.

I see that the official https://github.com/signalapp/libsignal doesn't have the Go implementation/binding unfortunately. I found some custom implementations https://libs.garden/go/search?q=libsignal but looks not well maintained. Maybe the C part of the Swift bindings could be used from Go as well but would need to have it on another repo first.

I already tried the webhook. But it currently does not support POST requests and not customer payload data.
Thus I can't use it right know.

Ah indeed we can't customize the payload atm.

On the other hand, isn't everything a webhook? I thought to have a more precise plugin might make more sense. But it is your project. I am also willing to modify the webhook to fit needs. Up to you?

Yes feel free to open another PR to be able to send a custom payload for the webhook notifier and we can take a look here if it fits your needs or requires a dedicated notifier.

@MrRagga-
Copy link
Contributor Author

I already tried to modify the custom webhook, but since I am not very familar with golang, I struggled what the best way is to support any abritary json data. Therefore I went with the native signal implementation, which was much easier. But if you can give me any pointer how to do it, I can submit a PR for the webhook, too.

@MrRagga-
Copy link
Contributor Author

The signal rest-api is quite a famous solution to use for Signal, e.g. https://github.com/louislam/uptime-kuma/blob/master/src/components/notifications/Signal.vue and https://www.home-assistant.io/integrations/signal_messenger/ both recommend to use it.

@crazy-max
Copy link
Owner

I already tried to modify the custom webhook, but since I am not very familar with golang, I struggled what the best way is to support any abritary json data.

I will take a look and open a PR that's fine.

The signal rest-api is quite a famous solution to use for Signal, e.g. https://github.com/louislam/uptime-kuma/blob/master/src/components/notifications/Signal.vue and https://www.home-assistant.io/integrations/signal_messenger/ both recommend to use it.

I see, then I'm ok with this notifier if you can add some docs. I will review after that.

@MrRagga- MrRagga- force-pushed the signal-rest-api branch 2 times, most recently from 6223244 to d382e47 Compare July 25, 2022 20:09
@MrRagga-
Copy link
Contributor Author

I added docs. Feel free to review and leave comments.

@crazy-max crazy-max closed this Oct 8, 2022
@crazy-max crazy-max reopened this Oct 8, 2022
Copy link
Owner

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@crazy-max crazy-max merged commit 42e684d into crazy-max:master Oct 8, 2022
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