Oh that’s even better, provided one can relatively easily escape <details> and <summary> tags that appear in the PR’s description when quoting them.
Thank you for creating workflows: Add a simple pull request subscription workflow by tstellar · Pull Request #64913 · llvm/llvm-project · GitHub
I agree that it will be much better than the current subscription-via-CODEOWNERS mechanism.
1 Like
Status update:
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.
7 Likes
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.
@jh7370 Yes, I think combining the mentions into a single comment would be better. I’m going to add this to the TODO list.
1 Like
1 Like
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.
1 Like
Status Update:
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.
3 Likes
Are there instructions for how to subscribe to a label? I did some quick googling but didn’t come up with anything.
Does this means I’ll stop receiving notifications on all groups I have subscribed? How can I see the mapping between the new tags and the old groups?
I can’t see any issue if I don’t know how to migrate to the new system, is there documentation on how to do that?
Shouldn’t we want until most people have made sure their workflows won’t be affected by this? Why the rush?
@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.
1 Like
You reviewed my patch that documented it just two days ago! 
LLVM Developer Policy — LLVM 18.0.0git documentation
=> 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.
That talked about joining teams, and that teams were associated with paths; not that teams were associated with labels?
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.
FWIW I have some notes how patch subscription works Reflections on LLVM's switch to GitHub pull requests | MaskRay
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 thehttps://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 thehttps://github.com/orgs/llvm/teams/pr-subscribers-*pages for pending join requests. So if they miss the email notification that sayswould 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 affectingclang/lib/Driver/XXwill receive the labelsclangandclang:driver, and thepr-subscribers-clangandpr-subscribers-clang:driverteams will be notified.
2 Likes
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)
1 Like
PR to remove the CODEOWNERS file: Remove CODEOWNERS file and copy missing paths into new-prs-labeler.yml by tstellar · Pull Request #66145 · llvm/llvm-project · GitHub
Do we plan to add it back with actually code owners?
If we continue receiving the same notifications, it’s fine.
But it’s still unclear to me how I wold “subscribe” to other/new labels.
You still join the teams. Talking about labels here is just an implementation issue as @mehdi_amini said above. If you are in the right teams nothing will change.
2 Likes
I think the libc++ developers want to do this, so likely yes.