search

python / miss-islington

🐍🍒⛏🤖 - A bot for backporting and merging CPython pull requests
Apache License 2.0
108 stars 38 forks source link

Repeated status messages on merged PR #577

Open ezio-melotti opened 2 years ago

ezio-melotti commented 2 years ago

In https://github.com/python/cpython/pull/96003#issuecomment-1215966581 miss-islington started reporting several status messages on a PR merged 24 days earlier. Not sure what triggered her.

cc @DanielNoord

Possibly-related issues:

DanielNoord commented 2 years ago

I had a quick look at the recent commits and don't think this was recently introduced. My last two PRs don't register any new listeners and also don't change the leave_comment check.

Thus, I think it is indeed a duplicate of the issues you mentioned.

One solution would be to check whether all checks finished and only leave a comment after that. For example, in the PR linked in #568 (https://github.com/python/cpython/pull/94849) some the comments state that the status is "pending". That's not really useful I think?

ambv commented 2 years ago

Personally I don't think those comments are useful anyway unless "automerge" is used, in which case it's the only way for us to really see what's going on so I think we should leave them on. In which case we still need to fix the bug there that makes the bot recomment on closed PRs, which it should not in any case. That's open to discussion. WDYT @Mariatta?

kumaraditya303 commented 2 years ago

+1 to get rid of those comments entirely and for "automerge" the bot should only comment when something went wrong not just spamming with "status check is done and success" and only on "open" PRs.

AlexWaygood commented 2 years ago

I agree that it would be nice to get rid of these comments entirely. And whatever the case, I think miss-islington should definitely stop tagging people in her comments. This makes it impossible for people to unsubscribe from a PR thread if she does start spamming it with messages.

ezio-melotti commented 2 years ago

My understanding is that miss-islington would merge PRs when the mandatory checks completed successfully, but since some of the optional checks might fail after the merge (when no one is looking at the PR anymore), then she would send the message to notify the people subscribed of the failure.

A message for each new failure helps people react quickly (instead of waiting for all the checks to complete), but it might end up being spammy if there are many optional checks that fail. However what's happening in the linked PR seems unrelated, since I only see one (old) failure and many messages.

DanielNoord commented 2 years ago

I'd be happy to work on this but there doesn't seem to be a consensus on the desired solution. Is there anything I can do to get such a consensus?