Add mentions. #1625
Add mentions. #1625
Conversation
| .paths | ||
| .iter() | ||
| // Only mention matching paths. | ||
| // Don't mention if the author is in the list. |
There was a problem hiding this comment.
We'll probably want to consider mapping this through GitHub teams, but not worth doing that right now.
src/handlers/mentions.rs
Outdated
| let client = ctx.db.get().await; | ||
| let mut state: MentionState = issue_data::load(&client, &event.issue, MENTIONS_KEY) | ||
| .await? | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Hm, so I'm a little worried that we can have pretty fast near-simultaneous events/pushes arriving, which means that the time between loading and saving the state could be significant. It seems like we'd ideally serialize around the primary key in the table -- maybe a postgres transaction is a good way to do that? Not sure.
A half-way OK solution is also to just stick a static mutex around this, which won't work on deploys but those are relatively rare (and so not a problem in most cases). Probably needs to be an async mutex.
That's the primary advantage of using the database for this -- we already have a rudimentary state mechanism via data_section in src/interactions.rs, which uses the issue/PR description. I think the database is probably a better mechanism for any state like this that's more automatically driven (rather than user interaction), and in general is easier to interact with and avoids polluting the issue description with garbage (even if theoretically hidden).
There was a problem hiding this comment.
Yea, that's probably wise. My postgres skills are pretty rusty, and I'm not sure if there is an elegant way to do that with this primitive JSON blob schema. I pushed an update that uses a crude "LOCK TABLE". There's some risk that concurrent events could cause significant delays, but I don't feel particularly worried about that.
There was a problem hiding this comment.
This looks okay to me. I think we could maybe use a row lock instead, but that's mostly just an optimization and doesn't seem worth doing right now (can be done later).
|
Thanks! I'm going to go ahead and merge this tomorrow, so I can be around to help resolve issues if something goes wrong. Not expecting any problems though. Once this is ready we can post a PR adding/migrating high-five mentions to triagebot and after a short period of duplicate mentions remove the high-five configuration for rust-lang/rust (and other repositories eventually). |
This adds a feature to add comments to PRs that touch certain files to ping interested people or to give information to the author. This is intended to take over the responsibility from highfive.
One of the key changes compared to highfive is that this can handle subsequent pushes to a PR (fixing rust-lang/highfive#223). To avoid duplicate mentions in that scenario, this PR introduces the concept of "issue data" which stores data about an issue in the Postgres database.