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

Repeated status messages on merged PR #577

Open
ezio-melotti opened this issue Sep 9, 2022 · 6 comments
Open

Repeated status messages on merged PR #577

ezio-melotti opened this issue Sep 9, 2022 · 6 comments
Labels

Comments

@ezio-melotti
Copy link
Member

In python/cpython#96003 (comment) 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
Copy link
Contributor

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 (python/cpython#94849) some the comments state that the status is "pending". That's not really useful I think?

@ambv
Copy link
Contributor

ambv commented Sep 9, 2022

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
Copy link

kumaraditya303 commented Sep 9, 2022

+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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

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?

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

No branches or pull requests

5 participants