Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
iritkatriel opened this issue Nov 27, 2022 · 12 comments

Comments

@iritkatriel
Copy link
Member

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.

@Mariatta
Copy link
Member

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.

@iritkatriel
Copy link
Member Author

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

@arhadthedev
Copy link
Member

arhadthedev commented Nov 27, 2022

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.

@iritkatriel
Copy link
Member Author

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.

@arhadthedev
Copy link
Member

Dead PRs should be closed.

Ultimately yes. However, it requires a wide discussion first as noted by Brett in python/cpython#21247 (comment). So, to get the useful notifier sooner, we need to add its limited version.

@iritkatriel
Copy link
Member Author

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?

@arhadthedev
Copy link
Member

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.

@terryjreedy
Copy link
Member

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.

@iritkatriel
Copy link
Member Author

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

Yes, absolutely agree.

@hugovk
Copy link
Member

hugovk commented Nov 28, 2022

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

@erlend-aasland
Copy link

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.

@hauntsaninja
Copy link
Contributor

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

I started a draft in python/bedevere#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants