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

Slack bridge: Update Slack bridge with Events API. #826

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

PieterCK
Copy link
Collaborator

@PieterCK PieterCK commented Jun 7, 2024

Currently, we use Slack's legacy RTM API as the "listener" for messages from Slack to Zulip. This PR updates our Slack Bridge to use the Webhook integration to send messages from Slack to Zulip.

We now utilize the Slack Webhook integration for this part of the bridge, which is being updated to use the latest Event API in PR#30465. Here are the key changes in this PR :

  • Updated the documentation on how to set up the updated Slack Bridge.
  • Removed the RTM API-based connection. Users can still use an older version of this repo if they want to maintain their existing connection.
  • Made the bridge compatible with the Slack Webhook integration by preventing looping messages.

new Slack Bridge documentation:
image

Fixes #825.

@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from d22ae40 to 3ca5796 Compare June 11, 2024 14:52
@zulipbot zulipbot added size: L and removed size: XS labels Jun 11, 2024
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch 4 times, most recently from 8ad1ab0 to 10bcf4f Compare June 22, 2024 13:00
@PieterCK PieterCK marked this pull request as ready for review June 22, 2024 13:15
@PieterCK
Copy link
Collaborator Author

@zulipbot add "buddy review"

@kenneth please review this please, thank you

@zulipbot
Copy link
Member

ERROR: Label "buddy review" does not exist and was thus not added to this pull request.

@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch 3 times, most recently from 679f982 to bc44575 Compare June 24, 2024 04:19
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Jun 26, 2024
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.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from add6643 to e682ef6 Compare June 26, 2024 08:19
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Jun 26, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@zulipbot zulipbot added size: XL and removed size: L labels Jun 26, 2024
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Jun 28, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from 42e472c to ded0ea7 Compare June 28, 2024 04:21
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Jun 29, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from eda7a33 to d8e6f71 Compare June 29, 2024 08:05
@PieterCK
Copy link
Collaborator Author

Update: Addressed review and dropped support for RTM API

@PieterCK
Copy link
Collaborator Author

PieterCK commented Jul 3, 2024

@sbansal1999 I think this one's also ready for mentor review, please do check it out! Thanks

PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Jul 3, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from d8e6f71 to e0f20f6 Compare July 3, 2024 09:04
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Oct 20, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from e0f20f6 to a394987 Compare October 20, 2024 11:22
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Oct 20, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from a394987 to da246c4 Compare October 20, 2024 11:38
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Oct 20, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from da246c4 to 5f42745 Compare October 20, 2024 11:43
PieterCK added a commit to PieterCK/python-zulip-api that referenced this pull request Oct 20, 2024
This is an optional commit that ignores linting errors
in an unrelated file from zulip#826.
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from 5f42745 to b4bcbf0 Compare October 20, 2024 11:50
@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch from b4bcbf0 to ff12189 Compare December 13, 2024 15:29
@PieterCK
Copy link
Collaborator Author

Now that PR#30465 is merged, this is ready for review!

@@ -5,30 +5,42 @@ This is a bridge between Slack and Zulip.
## Usage

### 1. Zulip endpoint
Copy link
Member

Choose a reason for hiding this comment

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

This likely needs to document this requires Zulip 10+, right?

Copy link
Collaborator Author

@PieterCK PieterCK Dec 14, 2024

Choose a reason for hiding this comment

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

Yes. I added this in the documentation header for clarification:

# Slack <--> Zulip bridge

This is a bridge between Slack and Zulip.

> [!NOTE]
> This setup requires at least Zulip version 10.0.

## Usage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

@PieterCK PieterCK force-pushed the issue-slack-bridge-update-825 branch 2 times, most recently from 65e0ca3 to ba9a9a1 Compare December 14, 2024 15:09
@PieterCK
Copy link
Collaborator Author

This is ready for review!

Slack Bridge now uses the Slack Webhook integration
to get messages accross from Slack instead of the
legacy RTM API based we preivouslt use.
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.

This prevents that by filtering out any messages from the Slack Webhook
bots when sending messages from Zulip to Slack..

Fixes zulip#825.
This commit updates the Slack Bridge doc, primarily guiding the user
to use our Slack Webhook integration.

With significant rewriting by tabbott.
@timabbott timabbott force-pushed the issue-slack-bridge-update-825 branch from ba9a9a1 to 67c8034 Compare December 16, 2024 19:55
@timabbott timabbott merged commit 67c8034 into zulip:main Dec 16, 2024
@timabbott
Copy link
Member

Merged, after some more rewriting of the docs and moving the documentation commit to the end.

Can you do some end-to-end testing of this end result and post about it in the #integrations channel? Once we've confirmed it's all happy, I can initiate a 9.4 server release and a release of this project to make this more readily available to end users.

@PieterCK
Copy link
Collaborator Author

Ok, I should be able to do that tomorrow. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slack Bridge: Discontinuation of classic Slack App
4 participants