trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.19k stars 565 forks source link

Many Trilinos developers getting spammed by Trilinos Master Merge PRs #12718

Open bartlettroscoe opened 7 months ago

bartlettroscoe commented 7 months ago

@trilinos/framework, @sebrowne, @achauphan CC: @rppawlo, @ccober6, @cgcgcg, @jwillenbring

Description

It seems that whenever the Trilinos system sets up a "Trilinos Master Merge PR Generator" PR, it will automatically assign a bunch of reviewers based on the packages with files being modified on the 'develop' branch compared to the last update of the 'master' branch. For example, for the recent master merge PR:

a bunch of different package teams got listed as reviewers:

image

The problem with this is that any Trilinos developer (e.g. me) that is on any of those teams will get spammed by emails about these master merge PRs. That is not good because no reviews should be necessary to for a merge of 'develop' to 'master'. If the tests pass, it should merge, period. No reviews of these PRs should be needed because any of the changes in 'develop' should have been fully reviewed when the PRs for these changes were merged into 'develop'. The only reviewers should be other Trilinos Framework members so that someone can do a rubber stamp approval so it can be merged. (Or if a automated review approval could be done, even better.)

Proposed solution

Do the following for these "Trilinos Master Merge PR Generator" PRs:

That would avoid spamming Trilinos developers about these 'develop' to 'master' merge PRs but the Trilinos framework team members will still be alerted if something goes wrong with the automated builds and tests. (Unless the "additive test assumption of branches" is violated by two or more of the topic-branches merged to 'develop', there should be no test failures, other than the standard set of random test failures experienced in Trilinos PR testing. So the AT: RETEST label should be applied until the builds and tests pass; taking into account the status of PR testing during that time period.)

bartlettroscoe commented 7 months ago

FYI: The only workaround I know for this problem is for every Trilinos developer getting spammed to click the "None" button under "Notifications" on these PRs. That should avoid future email spamming.

Update: Here is an example of the emails you get just when the PR is first created:

image

and then you get a bunch of emails as the autotester does its thing.

cgcgcg commented 7 months ago

I think we are added via a github mechanism and not by the script that creates the dev->master PR. We want to be auto-added when someone opens a PR against our packages. Not sure if this can be configured in a better way. Personally, I filter out these emails.

bartlettroscoe commented 7 months ago

I think we are added via a github mechanism and not by the script that creates the dev->master PR. We want to be auto-added when someone opens a PR against our packages. Not sure if this can be configured in a better way. Personally, I filter out these emails.

@cgcgcg, the problem with filtering out all of the emails that have "Trilinos Master Merge PR Generator" in the title is if there is a legitimate failure due to a violation of the "additive test assumption of branches" and someone put in an explicit @mention (e.g. @cgcgcg), then that person will never see that comment.

@trilinos/framework, @sebrowne, perhaps the tool that creates the dev->master PR can post-modify the PR to remove all of the reviewers that got added by the GH "code owners" feature that is populating these team reviewers? (I think GitHub has a REST API to do such things. Here is a python script created by Bard to do this.)

NOTE: I think this problem is going to persist with the move the GHA to run all of the PR builds. You will still be creating these dev->master merge PRs and GH will still be populating the PR with a bunch of code owner teams and spam Trilinos developers.

jhux2 commented 7 months ago

@bartlettroscoe Since MM happens just once per week, I don't find it too noisy. And if something happens that causes a lot of notifications, I just unsubscribe from the MM PR.

sebrowne commented 7 months ago

It would be nice if it didn't assign reviewers for an MR that is intended to be automated, so I agree with the concept of the issue/change request, but I also gree with the general sentiment of it being easy to ignore. Will bring this up at the next operational leadership meeting for prioritization and report back here.

csiefer2 commented 7 months ago

Or you could just turn off notifications ;P Or have your email client auto-trash them.

bartlettroscoe commented 7 months ago

If it is just once per week, that is not too bad. Every Trilinos developer just need to delete 4 emails and click "unsubscribe" 52 times a year.