Add Rocket.Chat integration #139
Conversation
rht
force-pushed
the
rocket.chat
branch
3 times, most recently
from
d1b6c09 to
dd61443
Compare
|
|
||
| ### 2. Rocket.Chat endpoint | ||
| 1. Create a user | ||
| 2. Enter the user's username and password into rocket_bridge_config.py |
There was a problem hiding this comment.
Is it rocket_bridge_config or rocket_mirror_config?
There was a problem hiding this comment.
Whoops, my mistake it should be rocket_mirror_config (using Zulip's nomenclature).
|
|
||
| # A bidirectional mirror | ||
| p1 = mp.Process(target=zulip_client.call_on_each_message, | ||
| args=(zulip_to_rocket(rocket_client, config),)) |
There was a problem hiding this comment.
It might make sense to extract a local here:
zulip_message_handler = zulip_to_rocket(rocket_client, config) # returns configured function handler
p1 = mp.Process(target=zulip_client.call_on_each_message, zulip_message_handler)
~~
| print("Starting message handler on Rocket.Chat client") | ||
| p2.start() | ||
|
|
||
| p1.join() |
There was a problem hiding this comment.
Is there a more idiomatic way to handle two forever-running subprocesses?
There was a problem hiding this comment.
Yes, there is only if one of the client has the feature to be non-blocking. So far, only Matrix does: https://github.com/rht/python-zulip-api/blob/b94bf2a31325243f0c4e5402087f17fca77da79e/zulip/integrations/matrix/matrix_bridge.py#L94.
There was a problem hiding this comment.
If zulip_client.call_on_each_message can be made non-blocking, then no multiprocessing is necessary.
There was a problem hiding this comment.
I actually have no problem with these being multiprocess, since the two processes are essentially independent and don't require any coordination. I'm more concerned about tearing them down, so it might make sense to use something like supervisord.
There was a problem hiding this comment.
Yeah, I'm concerned with the zombie processes even with p.join(). To remain lightweight, what about using multithreading like the Matrix client does[1]?
There was a problem hiding this comment.
I actually prefer multiple processes here.
|
Hello @rht, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly. Thank you for your valuable contributions to Zulip! |
4 similar comments
|
Hello @rht, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly. Thank you for your valuable contributions to Zulip! |
|
Hello @rht, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly. Thank you for your valuable contributions to Zulip! |
|
Hello @rht, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly. Thank you for your valuable contributions to Zulip! |
|
Hello @rht, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly. Thank you for your valuable contributions to Zulip! |
|
@rht A Zulip maintainer reviewed this PR over 10 days ago. Please take a look at the requested changes and update your PR, or leave a comment if you will not be able to do so soon. Thanks! |
This is a follow-up from https://chat.zulip.org/#narrow/stream/integrations/subject/Rocket.2Echat.
What is pending is the documentation on how to set this up, lol, since the IRC mirror is about as undocumented as this one.Documentation added.