klothoplatform/slack-notifier

Slack notification bot

slack-notifier's Issues

update title and description in slack if they change in PR

simplify botId logic in slack-listener

We currently have logic to get the botId with various methods and callbacks, but it's available already as an argument to the requests. Use that instead.

"framework" for unit tests

Really just some simple helpers and mocks, so that we can start writing good tests as we develop new features.

Add some cooldown or batching to comment messages to prevent spam

Copy/pasting the Slack convo for context and some ideas:

Gordon
@Yuval Shavit maybe some kind of CD for ^ for when multiple people are commenting at the same time
Yuval Shavit
CD?
Gordon
sorry, CoolDown - a timer that disallows similar actions until it has elapsed
Yuval Shavit
ah, gotcha. Yeah, the problem is that in a lot of cases you don’t want that. If I post a question and then you post an answer very quickly, and then I say “ah cool, makes sense” — then I think you probably do want to see that ack.
Definitely not opposed to it (and I even called out this kind of notification-spam in my short doc on comment notifications), but I think it needs some thought as to what the desired behavior is.
Gordon
I guess it depends on the scenario - I use the slack comments to be like "hey, look at the PR again". If there's a lot of back-and-forth, I'm most likely already looking at it
Yuval Shavit
Yep, I agree completely — the trick is when is it “a lot”, as opposed to “ask a quick question, get a quick response.”
I’m definitely not saying we can’t draw the line — just that I haven’t yet put much thought into where that line is.
Gordon
Yeah, I think it's 1x back-and-forth = 👍 , > a couple (2?) then start to throttle messages
Gordon
some kind of heuristic like that
Yuval Shavit
Some of the nicer heuristics might require new klotho functionality, and specifically the ability to set a delay on subsub notifications. That would let us take the notifications and put them on a queue to be processed in 30 seconds (or whatever), and in the meanwhile we’ll have a better sense of the history
Gordon
oh, maybe instead of "last commenter" it could do something more like batching (after the first message, to keep it responsive for the quick case)
Yuval Shavit
yeah, that batching is exactly the kind of thing I mean 🙂
Gordon
so after the first one, wait until no comments for X minutes then a
gordon-klotho commented Y times on the PR
Yuval Shavit
yup, 💯
Gordon
well, doesn't have to be a queue per se just some data store that we can add & remove from
Yuval Shavit
I actually think that would not only be a great feature for the bot, but also a great way to incorporate a new capability into the app!
Gordon
like a DB :)
Yuval Shavit
how would you implement the “[wait] X minutes then [action]“?
Gordon
polling
Yuval Shavit
yuck 😛
Gordon
why yuck?
Yuval Shavit
well firstly it’ll be a good deal more expensive — it means we need to use fargate instead of lambda. Secondly, it forces an infra decision (fargate instead of lambda). And thirdly, event-driven is just nicer imo.
Gordon
Well, you can add polling to lambda, just not through Klotho (yet)
Gordon
Event-driven is okay, but how do you "reset" the timer when you get a new message? There's not really a clean way to do that in event-driven
Yuval Shavit
as in have one fire off every 1 minute or so to collect all the actions you need? yeah, you could do that.
Gordon
event-driven could actually have worse perf depending on how rapidly the commenting happens (say 1 comment per 30s vs polling every 5 minutes)
Yuval Shavit
I was thinking that you’d put the info into the DB right away, and then a message to the queue to basically say “check back on this in 30 seconds.”
So ultimately, it’s still polling, just with a different mechanism.
Yuval Shavit
so I retract my yuck. point is, it would require some functionality that klotho doesn’t yet have 🙂
Gordon
would be a good impetus for implementing such
Yuval Shavit
agreed!

top-level message should include which repo

Basically:

--- PR #21: Adds NestJS + Sequelize sample app (+17426/-0) by DavidSeptimus-Klotho
+++ PR KlothoPlatform/sample-apps#21: Adds NestJS + Sequelize sample app (+17426/-0) by DavidSeptimus-Klotho

That does make it feel like a lot of link, though:

PR KlothoPlatform/sample-apps#21: Adds NestJS + Sequelize sample app (+17426/-0) by DavidSeptimus-Klotho

Maybe this?

KlothoPlatform/sample-apps PR#21: Adds NestJS + Sequelize sample app (+17426/-0) by DavidSeptimus-Klotho

add linter

https://typescript-eslint.io/docs/

When I did this, I got a bunch of errors and warnings. Clean them up as part of this.

initial setup and MVP

As little as is required to get a bot that:

1. Posts to slack when someone creates a PR
2. Replies to that slack message, and updates it with strikethrough, when the PR gets closed or merged

translate GH markdown to Slack mrkdwn

Today, we just post the GH-flavored markdown into Slack's mrkdwn. It works for some of the basic stuff, but also often breaks.

This library could help: https://github.com/jsarafajr/slackify-markdown (and by "help" I mean "basically do it all for us").

This would probably also fix #44.

notify when GH or Slack APIs have decreased performance

GitHub's webhooks are experiencing degraded performance right now, meaning that we're not getting notifications.

It'd be cool if the bot recognized that, and posted to the channel to give everyone a head's up.

Post to the slack thread when someone updates the PR with new pushes

bot doesn't correctly track PRs across org name changes

Let's say you had a repo at Acme/foo-repo, with a PR 123. If you rename Acme to WidgetMakers, then the bot loses track of the PR, and won't update its thread. It will just ignore all updates to that PR.

This is because we identify each PR by its URL, which includes the org name (afaik, there isn't any global PR identifier). When the org name changes, we can't find the original thread that the bot made, and without that it can't update or reply to it.

I believe this would apply to repo name changes too, though I haven't tested that.

hide previews when posting the top-level channel

If someone posts a PR for a public repo, Slack is able to pick up the page preview without any bot's help. It then attaches it to the message — and because the bot owns that message, we humans can't even remove it.

Hopefully there's a way to tell slack not to do that.

linkify GH's auto-linked things

See https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls.

The most important are probably:

• PRs/Issues:
  • #123 -> link to issue/PR in same repo
  • org/reponame#123 -> link to issue/PR in that repo
• Commits:
  • commit sha (full or abbreviated) -> link to commit in same repo
  • org/reponame@sha -> link to commit in that repo

Add commit messages for PR update messages

Proposal

before

KlothoBot PR updated: edb9442..0013b91

after

KlothoBot PR updated: edb9442..0013b91

• (351545a) Setup east provider according to AWS direction

Update the top-level slack post when someone re-opens a PR

Also add a comment in the thread

Configure which repos post to which channels

Today, we hard-code the channel id, and post updates from all repos to it.

It'd be nice to configure this on a per-channel basis. Something like:

/klothobot listen to CloudCompilers/klotho

and

/klothobot stop listening to CloudCompilers/klotho

and

/klothobot list repos

post a comment in the thread when someone leaves a comment

Create public landing page

don't post notifications for draft PRs

If event.pull_request.draft is true, then ignore it.

slack doesn't update the top-level message

This seems to be a client issue.

Repro (maybe?)

1. Put up a PR. The message will say it's ready.
2. Close the PR (merge, close, mark as draft — doesn't matter)
   1. expected: top-level message gets updated with new state
   2. actual: no change
      1. If anyone then replies in the thread, everyone will see the new, latest state

Seems to be that the client caches the top-level message.

We may be able to fix this just by having the bot first update the top-level message, and then post the "PR was merged" message to the thread. This may satisfy the 2.ii.a condition, and thus work around the issue.

Ping requested reviewers when added/posted

Need to find a way to correlate GH user -> Slack user (via email?).

Support when reviewers are added when the PR is posted as well as reviewers added after posting.

post a comment in the thread when someone posts a review

refactorings and small TODOs

• We keep specifying the top-level text over and over, with small variations. We should pull that out.
• Log the event's basic info (request id, PR number, etc) at the start of execution
• replace that stupid dispatcher with just await this["handle_${event.action}"](channel, event) (with safety checks if it's not found)
• Have onPrThread take as an arg a new SlackIO, which automatically adds the thread_id. I keep getting bitten by forgetting it in that context!
• Look for other opportunities to clean up, because there's a bunch of cruft in there already.

smarter decision around diff URL when pushing new commits to a PR

There are two ways to view a diff:

• https://github.com/klothoplatform/slack-notifier/SHA1..SHA2
• https://github.com/klothoplatform/slack-notifier/pull/215/files/SHA1..SHA2

The first one works if even you do the amend, but just shows the diff without context (ie without the PR it’s part of). The second shows it within the PR, but breaks if the left-side isn’t part of the PR’s current history — that is, if you squash/amended away a previous PR.

Ideally, the bot would be smart enough to use the PR-compare when it can, and the non-PR when it can't.

context: slack thread

`-` doesn't format as a list in slack

If my commit message has something like this:

- fizz buzz

Then GH renders it as a list:

• fizz buzz

... but Slack renders it as just the literal - text.

markdown in a title can the top-level post's links

For example, klothoplatform/sample-apps#31 has the following title:

add `npm run <klotho | pulumi>` scripts

This got rendered as:

PR <https://github.com/klothoplatform/sample-apps/pull/31%7C#31: add npm run <klotho | pulumi> scripts> (+777/-0) by yuval-klotho

%7C is |, weirdly enough.

Anyway, we should properly escape these things. Would be good to write a general markdown-to-slack conversion (including escaping any slack-specific sequences). Related to #12.

pin messages when the PR is opened, unpin when it's closed

Like this:

don't re-post message if `pr_opened` events gets redelivered

All calls should be idempotent.

If the PR already has a thread_ts stored, we should just return success.

We should similarly check to see if the description message has been posted, and only post it if it hasn't been (or maybe if there hasn't been any reply in the thread?)

need ability to customize emoji

We currently hard-code the emoji, which is bad on two counts:

1. It'd be nice to make it configurable; but more importantly
2. We're using custom emoji, so this will break for anyone who installs it without our emoji

We should:

1. Make the emoji selection configurable somehow
2. Default to standard slack emoji

Update the slack message when someone edits a comment on the PR

Depends on #6 and #7.