Enable link unfurling for Slack messages (with commit link support) #101
+350
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yasunariw
force-pushed
the
yasu/slack-unfurl-links
branch
from
b1abb0a to
76cb8bb
Compare
yasunariw
force-pushed
the
yasu/slack-webhook-to-api
branch
from
67f0996 to
6451321
Compare
yasunariw
force-pushed
the
yasu/slack-unfurl-links
branch
2 times, most recently
from
115dfb8 to
78e7077
Compare
yasunariw
force-pushed
the
yasu/slack-webhook-to-api
branch
from
6451321 to
cf9b42f
Compare
yasunariw
force-pushed
the
yasu/slack-unfurl-links
branch
2 times, most recently
from
d3462c8 to
82fb241
Compare
yasunariw
force-pushed
the
yasu/slack-webhook-to-api
branch
from
cf9b42f to
d527ce4
Compare
yasunariw
force-pushed
the
yasu/slack-unfurl-links
branch
from
82fb241 to
495382b
Compare
yasunariw
changed the title
yasunariw
changed the title
ygrek
approved these changes
Dec 30, 2020
yasunariw
force-pushed
the
yasu/slack-unfurl-links
branch
from
495382b to
f961df5
Compare
yasunariw
force-pushed
the
yasu/slack-webhook-to-api
branch
from
6e18496 to
7404d4a
Compare
yasunariw
force-pushed
the
yasu/slack-unfurl-links
branch
from
f961df5 to
3200af0
Compare
The new file `slack_message.ml` should only contain generation logic for messages that describe the state of some info on GH (e.g., a description of a PR), and *not* an action taken on GH (e.g, "XYZ opened a PR". This decoupling is desired because we eventually want to reuse code between link unfurl generation and event notification generation, and link unfurl only concerns the former type of info described above. Currently, it's difficult to do as there is tight coupling in `slack.ml`. Currently we support commit link unfurls. We can later easily expand the selection of unfurlable links by adding onto `slack_message.ml`.
Link unfurling requires usage of Slack's Events API. The first step in configuring it is url verification. Subsequently, we can start accepting event callback notifications, one of which is chat_unfurl.
Again, currently supports commit links but can be extended later on. We shouldn't unfurl if three or more links are present, or if the link type isn't supported for none of the urls.
Check for different possible url schemes and other variations.
yasunariw
force-pushed
the
yasu/slack-unfurl-links
branch
from
3200af0 to
cfc4921
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the task
(depends on #94)
This PR creates an initial thin pipeline for adding link unfurling functionality to Slack messages. We support commit links for now, with the possibility of extension to other link types (e.g., issues, PRs) down the road.
One caveat is that currently, Slack message generation code for unfurled links and event notifications reside in separate files (
slack_message.mlandslack.mlrespectively). This is because the latter message format tightly couples description of an action on a GH resource (e.g., "opened a PR", "pushed commits", "merged issue") with description of the resource itself (e.g., "a PR w/ N comments", "commit xyz with 5 changed files"). Since unfurled messages only need the latter, we can't take advantage of the existing code inslack.mlelegantly. A near-future goal should be to share more code between them. (Code reuse seems to be a concern from before -- #54)Users first need to configure their app to support the unfurling. Part of it is to support Slack's Events API, which this PR implements.
Like GH's official integration, we don't unfurl when a message contains three or more links.
How to test
This shouldn't affect existing tests. New tests for url pattern matching are defined in
tests/github_link_test.ml. You can also test by posting messages w/ links to a channel the bot is a member of.References