Another issue I’ve observed with the current approach is that when someone in the group approves the PR, it is approved “on behalf of ” and the group is removed from the reviewers and replaced with the person approving it [1]. I don’t know if the group still gets notifications afterwards, but it made it really confusing to see the PR disappear after being approved.
I’ve discovered that it’s possible to hide comments, so we could encode extra information when we tag a team as @jrtc27 has suggested.
With this, I think we can mitigate the biggest downside of the label based system which is the lack of information in the initial notification. I’m going to try to implement my proposal and submit a pull request. Please let me know if you see any other potential issues with this.
The label based notification system has been implemented now and is up and running. The last thing left to do is to make sure the team names match the label names. This has been done for some teams, but not all of them, so you may notice that new labels are generating mentions to some teams and not others. Once this last step is complete we will have something that can replace the CODEOWNERS file.
If everything goes well, we should be ready drop the CODEOWNERS file. I know some people may still want to use this file to help assign reviewers to PRs, but I think we should delete the existing file and start over with a discussion about how best to use it.
There are still a few known issues with the subscription system that we need to address:
Sometimes people mistakenly create pull requests with hundreds of commits that include changes already in main. This leads to a lot of notification spam as many teams end up being subscribed to these PRs.
Our bot user has been getting rate limited by GitHub. I haven’t seen this affect our subscription system yet, but it has affected the emails going to the commits list.
Thanks to everyone that has been helping with patches and suggestions, we made good progress this week.
This is definitely annoying! One thing I noticed though is that every team was mentioned in a separate email. Is this really necessary? Whilst obviously the “oops I subscribed all the teams because I forgot to rebase” is hopefully not a long term issue, it isn’t ideal that every team who SHOULD be mentioned will get so in a separate email too, as it’s still quite noisy.
I don’t mind the proposal and agree with most comments in the thread already, I’m just worried what we mean by “who have the authority to approve commits for a specific area of the project”.
I’m all for the idea that some people are more suited than others to approve the patch in some areas of the code, but we have so far struggled with the ideas to create such a separation. The current review policy does not distinguish either.
So far, our loose definition of approval has been “working” for the most part and people are generally civil about not approving code that is not in their area of expertise. So if we move into something more well defined, we’ll definitely need to update the review document.
In Phab, the “definition” of “who can approve a PR” was: “anyone who has a user in Phab”, which doesn’t even need to be a committer, but it had the gate keeping that a committer needed to push the code.
In Github, with codeowners files and teams we have basically the same setup, since someone with write access still has to merge the PR after being approved, so I don’t see it as a new problem. If anything, it’s an old problem that we haven’t fixed.
The proposal to separate people in three groups per directory (code owners, approvers, reviewers) will not only add maintenance cost (keeping those lists up to date) but friction (who decides in which list you can be, per directory?).
If this is the road we’re taking, then this has to be crystal clear on the policy before we make any change to the mechanics.
To be clear, I don’t want to derail the rest of this discussion, I just want to make sure we separate the mechanical discussions in this thread from this policy discussion, that should be taken in a separate RFC with the appropriate title and description, so that people can collaborate on the topic directly.
Everything is in place for the label based notifications, and it looks like it is working. If you see any issues, please let me know. I’m going to plan to remove the CODEOWNERS file later today if things are still looking good.
@rengolin You don’t have to do anything different. We are reusing the same teams as we had for CODEOWNERS.
I think we should move quickly, because it’s still early and we don’t want more people setting up their workflows around using the CODEOWNERS file. Based on this discussion, there are no advantages to keeping our current system, so I think we should move to something better ASAP.
=> there is no builtin support, we continue to use “GitHub teams”, nothing changes.
The only difference is that the teams aren’t added as “reviewers” of the PR, but instead a comment is added and the teams are @pr-subscribers-<team> notified.
I strongly support that we remove pr-subscribers-* from .github/CODEOWNERS and do it quickly, to prevent confusion and prevent users from relying on this work flow.
I think people can understand that things are a bit in flux as we started to use pull requests.
People who want new teams can update .github/new-prs-labeler.yml instead and check the existing labels and https://github.com/orgs/llvm/teams/issue-subscribers-* teams.
To enable component-based subscription, the llvm organization on GitHub has created multiple pr-subscribers-* teams, which users can freely join them. ( Note: A maintainer is supposed to accept every join request. Unfortunately, GitHub only displays the “pending request” information on the https://github.com/orgs/llvm/teams/pr-subscribers-* page and not on any other page. It’s not reasonable to expect a maintainer to routinely check the https://github.com/orgs/llvm/teams/pr-subscribers-* pages for pending join requests. So if they miss the email notification that says would like to join "LLVM", the request may remain pending indefinitely. )
Then we use label-based subscription. A GitHub action is set up to label every pull request, and these labels are used to notify pr-subscribers-* teams. For example, a pull request affecting clang/lib/Driver/XX will receive the labels clang and clang:driver, and the pr-subscribers-clang and pr-subscribers-clang:driver teams will be notified.
Ah, labels are just used as an implementation detail here: this is why the paths are stored in the file new-prs-labeler.yml.
A label is added, and then the GitHub action is handling it to @ the right team, so: join the team to subscribe to the label!
(this is what we were doing for issues until GitHub allowed to subscribe to issues based on labels)
