Skip to content
This repository has been archived by the owner on May 20, 2024. It is now read-only.

Prevent notification storms on corrupted/improper PRs #787

Closed
dagwieers opened this issue Oct 12, 2017 · 7 comments · Fixed by #825
Closed

Prevent notification storms on corrupted/improper PRs #787

dagwieers opened this issue Oct 12, 2017 · 7 comments · Fixed by #825

Comments

@dagwieers
Copy link
Contributor

dagwieers commented Oct 12, 2017

So almost daily we get some broken PRs with lots of existing commits in a new PR. Often the owner of the branch did something wrong and has no clue to resolve it and eventually just closes the ticket. But the real damage is already done at the time, as everybody involved with any of these commits (directly or indirectly) are getting mailed by ansibot.

Examples:

Either we should be able to identify these corrupt PRs, or at least prevent to send out notifications to more than an arbitrary number of people e.g. 25 ? Maybe check existing cases for a good treshold ?

@pilou-
Copy link
Contributor

pilou- commented Oct 18, 2017

Is more than 30 commits and more than 30 files modified a good filter for these PR ?

@cben
Copy link

cben commented Nov 29, 2017

Proposal: a simple "more than N people at once" limit for automatic notification.

If more than that, ansibot instead of mentioning all the people would explain that it suspects it's a mistake, and will list the usernames it would have cc'd (without @, or within backticks literal), so humans can (A) understand what's happening (B) manually cc all or some people if really intended (eg. large cross-cutting refactoring).

@jctanner
Copy link
Contributor

first implementation could be to disable all notifications if a merge commit is detected (something we already check for).

@dagwieers
Copy link
Contributor Author

Another common case recently we also have to consider:
ansible/ansible#33448 (comment)

@dagwieers
Copy link
Contributor Author

We are now closing one specific case, but I'd like to also consider a more general solution for other causes (like the one in my previous comment). I want to prevent any more of these cases because they make people want to drop out of any notifications.

@gundalow
Copy link
Collaborator

gundalow commented Jan 3, 2018

Seems to be back. opened #837

@dagwieers
Copy link
Contributor Author

ansible/ansible#34011

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

Successfully merging a pull request may close this issue.

5 participants