Add merge-conflict notifications #1856
Conversation
|
Full support for having merge conflict warnings, love it ❤️. On a more technical note, have you been able to test it? |
|
Yes, here was my test plan:
|
src/handlers/merge_conflict.rs
Outdated
| // Spawn since this can trigger a lot of work. | ||
| tokio::task::spawn(async move { | ||
| // See module note about locking. | ||
| let gh = GithubClient::new_from_env(); |
There was a problem hiding this comment.
Is there a reason we need a new client vs. using the existing ctx.github client? Seems like that should be Cloneable?
There was a problem hiding this comment.
Yea, that sounds good! For some reason I was just assuming it wasn't cloneable (or maybe I was concerned about how the connection pooling would work?).
src/handlers/merge_conflict.rs
Outdated
| // row-lock held between the time it is loaded and the save call below. | ||
| // The `post_comment` call should normally be pretty fast, so races should | ||
| // be rare. If they happen too often, consider adding some locking | ||
| // mechanism here. |
There was a problem hiding this comment.
I think there is a lock held, actually on the full table? load(...) starts with LOCK TABLE issue_data and presumably that's kept until the transaction ends (which happens in save).
triagebot/src/db/issue_data.rs
Line 41 in 742b66b
Do we think we need more?
There was a problem hiding this comment.
I don't remember what I was thinking at the time. I saw the LOCK TABLE call, but for some reason was thinking there was something else. I can't think of what it is now, and the lock is clearly held in the IssueData, so I removed the comment. Thanks for pointing that out!
|
Generally onboard with landing mostly as-is, a few questions/comments. |
Co-authored-by: Mark Rousskov <[email protected]>
I don't remember what I was thinking at the time. It looks like the transaction is stored in IssueData, so the lock should persist.
The client is already cloneable and should reuse the connection pool.
Personal preference, felt better this way.
ehuss
force-pushed
the
merge-conflict
branch
from
90b7023 to
5301321
Compare
This adds a new handler which will post comments on PRs when there is a merge conflict. I have appreciated this feature in homu, and I think it would be helpful for repositories that are not using homu.
A high-level overview:
[merge-conflict]conflg table to enable the feature. It supports labels just like homu.mergeablestatus of a PR is updated both lazily and asynchronously. That is, GitHub doesn't bother computing it until you ask for it, which it then returnsnull. Then, GitHub has an async background task which computes the value. There is no notification (AFAIK) when that is done, so we just poll again after a delay.