core-workflow icon indicating copy to clipboard operation
core-workflow copied to clipboard

bot should ping author when PR has merge conflicts and mark the PR as "awaiting changes"

Open iritkatriel opened this issue 3 years ago • 13 comments

When a PR has merge conflicts, the author doesn't know unless they check or another person alerts them.

It would be good if the bot could ping them and move the PR to "awaiting changes" state.

iritkatriel avatar Nov 27 '22 11:11 iritkatriel

Let's try doing this as a GH Action instead of in bedevere.

I think this would require checking the PR's "mergeable" status on the "synchronized" or "push" event.

Mariatta avatar Nov 27 '22 15:11 Mariatta

I think a nightly job checking each open PR would be sufficient, and possibly simpler?

iritkatriel avatar Nov 27 '22 15:11 iritkatriel

I think a nightly job checking each open PR would be sufficient, and possibly simpler?

Probably, the job should skip inactive PRs (that have the last message older than, say, three month). It's vital to prevent piling up everyone's unread notifications with hundreds of dusted patches.

arhadthedev avatar Nov 27 '22 15:11 arhadthedev

I think a nightly job checking each open PR would be sufficient, and possibly simpler?

Probably, the job should skip inactive PRs (that have the last message older than, say, three month). It's vital to prevent piling up everyone's unread notifications with hundreds of dusted patches.

Dead PRs should be closed.

iritkatriel avatar Nov 27 '22 15:11 iritkatriel

Dead PRs should be closed.

Ultimately yes. However, it requires a wide discussion first as noted by Brett in https://github.com/python/cpython/pull/21247#discussion_r449228875. So, to get the useful notifier sooner, we need to add its limited version.

arhadthedev avatar Nov 27 '22 16:11 arhadthedev

I'm not suggesting to automatically close PR. My point is that if you get pinged a lot about merge conflicts on your open PRs then I think you're probably doing something wrong. How many open PRs should a person have on the go?

iritkatriel avatar Nov 27 '22 17:11 iritkatriel

My point is that if you get pinged a lot about merge conflicts on your open PRs then I think you're probably doing something wrong.

Ping messages are not private so they're sent to everyone subscribed to python/cpython.

However, if notifications would be emailed from python.org to an address in the first commit of a PR, my concerns are irrelevant.

arhadthedev avatar Nov 28 '22 00:11 arhadthedev

There are 44 open PRs with Idle in the title. Yes, that is too many. Currently, it would be useless to ping anyone but me, as the assignee. I would only want 1 ping per PR with a conflict. If a PR is marked with a conflict, do not ping again. Ideally, I would like a way to test whether a PR causes conflicts before merging, or even before making a PR, so I could consider which order to do them in.

terryjreedy avatar Nov 28 '22 04:11 terryjreedy

If a PR is marked with a conflict, do not ping again.

Yes, absolutely agree.

iritkatriel avatar Nov 28 '22 08:11 iritkatriel

Duplicate of https://github.com/python/core-workflow/issues/96 ("Add a bot to notify people when their PR has a merge conflict")?

hugovk avatar Nov 28 '22 12:11 hugovk

Duplicate of #96 ("Add a bot to notify people when their PR has a merge conflict")?

Yes, it seems like this is a duplicate. Suggesting to close either the old or this issue as a duplicate.

erlend-aasland avatar Nov 28 '22 12:11 erlend-aasland

I'm interested in picking this up. Adding a label seems like a good place to start.

I started a draft in https://github.com/python/bedevere/pull/530. On every push to main, we requery mergeability status for each open PR. Using GraphQL means we can do this in relatively few requests, so we don't need to worry too much about rate limits.

hauntsaninja avatar Jan 06 '23 10:01 hauntsaninja