spack / spackbot

Spack maintainer bot 🤖
https://spack.github.io/spackbot/
Other
8 stars 12 forks source link

Notify maintainers for PRs that modify large number of packages #59

Open adamjstewart opened 2 years ago

adamjstewart commented 2 years ago

When I first implemented spackbot, I added a guard in the code that prevents maintainers from being notified on PRs that modify 100+ packages. The idea was that PRs that affect that many files are often automated changes like license updates where we don't actually need to notify anyone.

Recently, we've had a few very large PRs where I would like to bypass this guard and notify maintainers:

The first modifies 300+ packages, while the latter two actually modify < 100 files and should work, but aren't working. So this is also a bug report that @spackbot maintainers doesn't seem to be working correctly.

I think the ideal behavior would be for PRs modifying 50+ packages to not add maintainers by default, but if someone runs @spackbot maintainers it should add maintainers regardless of how many files are modified. Does this seem reasonable? Is this doable?

Also, GitHub has a limit on the number of people you can mention in a single comment (seems to be 50) so we may need to break these into multiple comments. Although over time more people will be added as triage and will get assigned as reviewers instead of being included in the comment, so maybe this is a non-issue. It's a corner case at best.

scottwittenburg commented 2 years ago

Today I took a look through the logs to see if there were any hints as to why sometimes spackbot automatically adds maintainers, and sometimes it doesn't. I didn't find any clear evidence of a cause, but spackbot has run afoul of github's webhook timeout in other cases, and a couple of git commands are run as a part of the search for maintainers, so I think it could make sense to search for and add maintainers in a worker process, once #58 is merged.

vsoch commented 2 years ago

Personally I think having spackbot check/fix style (and making the application much more complex and then need to be able to read and respond to checks) might have been a decision that makes it overall more unstable because it's just so many events going in.