-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Slack bridge: Update Slack bridge with Events API. #826
base: main
Are you sure you want to change the base?
Conversation
d22ae40
to
3ca5796
Compare
8ad1ab0
to
10bcf4f
Compare
ERROR: Label "buddy review" does not exist and was thus not added to this pull request. |
679f982
to
bc44575
Compare
The github action running the tests uses Python 3.8.18. When running ./tools/run-mypy using this Python version there will be linting errors related to importing the 'Salesforce' class. However, this is not a problem when running the mypy linter under Python 3.10. This commit ignores these linting error just so that the main PR zulip#826 can pass the github actions tests. I wouldn't recommend merging this commit to permanently fix this issue. Instead, it might be better to update the github actions to use a more recent Python version.
add6643
to
e682ef6
Compare
This is an optional commit that ignores linting errors in an unrelated file from zulip#826.
This is an optional commit that ignores linting errors in an unrelated file from zulip#826.
42e472c
to
ded0ea7
Compare
It starts with "xoxp-...". | ||
|
||
4. In the `slack` section of the Zulip-Slack bridge configuration file, enter the bot name | ||
(e.g "zulip_mirror"), token (e.g xoxp-...), and the channel ID (note: must be ID, not name). |
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.
Optional: IIRC xoxb- (not xoxp-) is the legacy token? We could document this, to explicitly say that it is legacy.
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.
Correct, the new token is xoxb. Updated the doc 👍
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.
I had this in mind, might as well say it: what if we add detection of the token prefix in the code, that gives a clear error message if user specifies an xoxp- token.
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.
hmm, yeah after reviewing the setup instructions again I feel like there is a lot of room for human error. Adding error messages like this would help reduce the probability.
This commit updates the Slack Bridge doc, primarily guiding the user to use our Slack Webhook integration and reformating the old instructions to a separate legacy section.
This is an optional commit that ignores linting errors in an unrelated file from zulip#826.
eda7a33
to
d8e6f71
Compare
Update: Addressed review and dropped support for RTM API |
@sbansal1999 I think this one's also ready for mentor review, please do check it out! Thanks |
Slack Bridge now uses the Slack Webhook integration to get messages accross from Slack instead of the legacy RTM API based connection our Slack Bridge use. This commit adds a "--legacy" argument to the script, it acts as a toggle to run the RTM API based connection to get messages accross to Zulip. It is used to ensure backwards compitability for users who want to maintain any ongoing Slack mirror.
When using Slack Webhook integration to get messages from Slack to Zulip, we don't want to send back messages from the Slack integration bot. Fixes zulip#825.
This is an optional commit that upgrade the Python version for github workflows when running tests. This commit doesn't try to drop support for Python 3.8.
This is an optional commit that ignores linting errors in an unrelated file from zulip#826.
This is an optional commit that drops support running the Slack Bridge using the legacy RTM API. The reasoning for removing backwards compitability is because the user can just use the older version of pyton-zulip-api.
d8e6f71
to
e0f20f6
Compare
Currently we use the Slacks legacy RTM API as the "listener" from Slack to Zulip. This commit updates our Slack Bridge to use Webhook integration to get messages across from Slack to Zulip.
We use our Slack webhook integration for that part of the bridge, it is being updated to use the latest Event API at PR#30465. So, instead the changes in this PR are only the following:
Fixes #825.