workflows: Add a simple pull request subscription workflow #64913

tstellar · 2023-08-23T00:25:00Z

This new workflow will make it possible for people to subscribe to pull requests based on the labels that are added. Labels will be added automatically to the pull requests based on the modified files and each label will be associated with a GitHub team that will be notified when the label is added.

See https://discourse.llvm.org/t/changes-to-pull-request-subscription-system/73296

github-actions · 2023-08-23T00:25:13Z

This repository does not accept pull requests. Please follow http://llvm.org/docs/Contributing.html#how-to-submit-a-patch for contribution to LLVM.

tstellar · 2023-08-23T00:38:00Z

Example pr-subscribers team: https://github.com/orgs/llvm/teams/pr-subscribers-mips

joker-eph · 2023-08-23T04:06:08Z

Why aren't we using the native GitHub CODEOWNER files for subscription?

Right now something quite annoying with the issue subscription, which will also happens with the action is that the subscription does not send the initial email containing all the information. What I'm looking for with an auto-subscription to pull-request is a notification with the title, the description, and ideally 500 lines of the diff!

MaskRay · 2023-08-23T07:32:39Z

Why aren't we using the native GitHub CODEOWNER files for subscription?

Right now something quite annoying with the issue subscription, which will also happens with the action is that the subscription does not send the initial email containing all the information. What I'm looking for with an auto-subscription to pull-request is a notification with the title, the description, and ideally 500 lines of the diff!

Using a text file in the repository doesn't scale and adds unneeded churn to the repository.
We cannot afford a new commit every time someone wants to change their subscription rules.

tstellar · 2023-08-23T15:34:08Z

Right now something quite annoying with the issue subscription, which will also happens with the action is that the subscription does not send the initial email containing all the information. What I'm looking for with an auto-subscription to pull-request is a notification with the title, the description, and ideally 500 lines of the diff!

@joker-eph Is this what Phabricator does?

tstellar · 2023-08-23T15:38:07Z

@joker-eph @MaskRay We could do a hybrid approach where we use CODEOWNERS but instead of putting individuals in the file, we create teams (like I'm proposing here) and specify the teams as the code owners in the files.

The main difference also between using CODEOWNERS and what I'm proposing is that with the CODEOWNERS file, people are added as reviewers and with what I'm proposing, people are just subscribed to the PR.

joker-eph · 2023-08-23T17:37:57Z

@joker-eph Is this what Phabricator does?

Yes, here is an example of the email Phab sends initially, and it includes all Herald subscribers (and has all the infos you needs about the revision without opening Phab, on GitHub you'd need to open the link and an extra click to get the list of paths and the diff I believe):

Using a text file in the repository doesn't scale and adds unneeded churn to the repository.
We cannot afford a new commit every time someone wants to change their subscription rules.

I shared the concern, but ultimately I care more about my code reviewing experience, if we have to talk about things "we cannot afford", I would say "we cannot afford a sub-par code review workflow" first.

tstellar · 2023-08-23T19:10:41Z

I've done some testing and using the CODEOWNERS file does generate an email with the full PR summary:

[llvm_temp-issue-tester] Update README.md (PR #51).pdf

I think if we use teams rather than individuals in the file, it's going to be much easier to manage.

joker-eph · 2023-08-24T00:17:31Z

That seems fine to me! Are we gonna reuse the teams used for issues or have new ones?

tstellar · 2023-08-24T02:01:37Z

That seems fine to me! Are we gonna reuse the teams used for issues or have new ones?

I was thinking we would create new ones, just in case people just want to subscribe to pull requests or issues only.

asl · 2023-08-25T23:58:47Z

That seems fine to me! Are we gonna reuse the teams used for issues or have new ones?

Given that label-based issue notifications & subscriptions work now, we'd likely deprecate issue teams

smeenai · 2023-08-28T19:32:19Z

CODEOWNERS will subscribe you to an entire directory, right? Is there any solution for subscribing only to specific paths in a directory?

boomanaiden154 · 2023-08-28T20:07:45Z

CODEOWNERS will subscribe you to an entire directory, right? Is there any solution for subscribing only to specific paths in a directory?

You can use the same exact syntax that you can in a .gitignore, so you can definitely subscribe only to specific paths in a directory. How that would work with subscribing teams/avoiding code churn from individuals updating their rules probably needs more discussion though.

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners

This new workflows will make it possible for people to subscribe to pull requests based on the files that are being modified. Whenever a pull request is created, this action will look through all of the teams whose name begins with pr-subscribers- and it will try to match the files changed in the pull request to a comma separated list of globs in the team's description. One limitation of this workflow is that it only runs when a pull request is created, so any files added in subsequent updates will not create notifications for the appropriate teams. This is planned to be fixed in a future update.

The labels are added based on the file paths so we still ultimately end up with file path based subscriptions, but using labels as an intermediary gives us more flexibility.

joker-eph · 2023-09-08T04:21:54Z

llvm/utils/git/github-automation.py

+                patch = requests.get(self.pr.diff_url).text
+            except:
+                patch = ""
+            comment = "@llvm/{}".format(team.slug) + "\n\n<details><summary>Changes</summary><pre>\n" + patch + "\n</pre></details>"


If this shows up in the email, that's just gonna be great :)

Can we do it for issues when we're done with PR?

Yes, I tested and it shows up in the email notification. You do have to click in the email to expand the patch (if you are looking at the html version) like you do on the comment, but hopefully that's acceptable.

Can you file an issue for what format you want in the issue notifications?

Does the patch variable contain ```? If not, maybe add ```diff and ```

@MaskRay Do you mean the patch text?

Yes. Sorry for the typo, I mean whether the variable patch = requests.get(self.pr.diff_url).text contains ```

It won't normally have ```, but it could if one of the lines changed in the patch had it.

I think the pre tag will prevent the markdown from being rendered if that is what your going for with the ``` suggestion.

You are right. <pre></pre> is sufficient. Resolved I think.

Another nice-to-have thing is to list modified files before showing diff --git.

Maybe list the diffstat?

OK, I'm not sure exactly how to do that, but I'll add it to my TODO.

tstellar · 2023-09-08T04:24:58Z

I rewrote this so now labels are added to PRs based on the files changed by the PR, and then teams are notified with an @ mention based on the labels. I also added a diff of the PR to go along with the mention, so subscribers can see the context in their email without having to click on the PR link.

Here is an example of what the comment/notification will look like.

joker-eph · 2023-09-08T04:30:29Z

llvm/utils/git/github-automation.py

+            if self.team_name != team.name.lower():
+                continue
+            try:
+                patch = requests.get(self.pr.diff_url).text


Can this be arbitrarily large? Shall we limit the size here with a subset?
I don't know the limit for a comment size with GitHub API but I would be afraid of the comment posting failing because of that.

It seems like the comments are limited to 65536 characters. We can try to put some kind of limit on this.

joker-eph

This is a great improvement, if the email notification works I'd say ship-it and refine!

MaskRay · 2023-09-08T05:25:24Z

llvm/utils/git/github-automation.py

+        self.repo = github.Github(token).get_repo(repo)
+        self.org = github.Github(token).get_organization(self.repo.organization.login)
+        self.pr = self.repo.get_issue(pr_number).as_pull_request()
+        self._team_name = "pr-subscribers-{}".format(label_name).lower()


There is some discrepancy between the label names and the pr-subscribers- team names.

For example, backend:X86 vs pr-subscribers-x86, and the issue-subscriber name https://github.com/orgs/llvm/teams/issue-subscribers-backend-X86 .

That said, this is a small issue. I think we can rename pr-subscribers- team names to improve consistency.

@MaskRay Yes, we are going to have to create new teams. I think in this case what we will do is to make pr-subscribers-backend:X86 a parent team of pr-subscribers-x86, so when you mention pr-subscribers-backend:X86, members of pr-subscribers-x86 will be mentioned too.

I have a python script mapping the current team names to the new names, so we should be able to fix this for every team.

Sounds good! Thanks for the effort.

MaskRay · 2023-09-08T05:27:55Z

Thank you for creating this patch and starting https://discourse.llvm.org/t/changes-to-pull-request-subscription-system/73296 . I only got to understand how this patch will help the situation after reading the Discourse post. This looks great to replace the current pr-subscribers via .github/CODEOWNERS mechanism.

MaskRay · 2023-09-08T05:33:29Z

Consider linking to https://discourse.llvm.org/t/changes-to-pull-request-subscription-system/73296 in the description.

tru

LGTM. Make sure you run black on the python file before submitting.

jrtc27 · 2023-09-08T18:46:59Z

Please make sure this is limited to llvm/llvm-project like other issue-related workflows, otherwise forks will start running this action

github-actions bot closed this Aug 23, 2023

github-actions bot locked and limited conversation to collaborators Aug 23, 2023

tstellar reopened this Aug 23, 2023

cor3ntin added the github-prs label Sep 2, 2023

tstellar added 2 commits September 7, 2023 11:21

Rewrite to key notifications off of labels instead of file paths

2999769

The labels are added based on the file paths so we still ultimately end up with file path based subscriptions, but using labels as an intermediary gives us more flexibility.

tstellar force-pushed the pr-subscriber branch from b16d046 to 2999769 Compare September 8, 2023 04:17

tstellar requested a review from a team as a code owner September 8, 2023 04:17

tstellar requested a review from cor3ntin September 8, 2023 04:21

joker-eph reviewed Sep 8, 2023

View reviewed changes

joker-eph approved these changes Sep 8, 2023

View reviewed changes

Limit patch size to 20,000 characters

3ed0317

MaskRay reviewed Sep 8, 2023

View reviewed changes

MaskRay approved these changes Sep 8, 2023

View reviewed changes

tru approved these changes Sep 8, 2023

View reviewed changes

tstellar added 2 commits September 8, 2023 08:04

Run black on github-automation.py

0405eac

Drop unneeded import

5b74241

tstellar merged commit 5f16a3a into llvm:main Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workflows: Add a simple pull request subscription workflow #64913

workflows: Add a simple pull request subscription workflow #64913

tstellar commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023

tstellar commented Aug 23, 2023

joker-eph commented Aug 23, 2023

MaskRay commented Aug 23, 2023

tstellar commented Aug 23, 2023

tstellar commented Aug 23, 2023

joker-eph commented Aug 23, 2023 •

edited

Loading

tstellar commented Aug 23, 2023

joker-eph commented Aug 24, 2023

tstellar commented Aug 24, 2023

asl commented Aug 25, 2023

smeenai commented Aug 28, 2023

boomanaiden154 commented Aug 28, 2023

joker-eph Sep 8, 2023

tstellar Sep 8, 2023

MaskRay Sep 8, 2023 •

edited

Loading

tstellar Sep 8, 2023

MaskRay Sep 8, 2023

tstellar Sep 8, 2023

tstellar Sep 8, 2023

MaskRay Sep 8, 2023

tru Sep 8, 2023

tstellar Sep 8, 2023

tstellar commented Sep 8, 2023

joker-eph Sep 8, 2023

tstellar Sep 8, 2023

joker-eph left a comment

MaskRay Sep 8, 2023

tstellar Sep 8, 2023

MaskRay Sep 8, 2023

MaskRay commented Sep 8, 2023

MaskRay commented Sep 8, 2023

tru left a comment

jrtc27 commented Sep 8, 2023

workflows: Add a simple pull request subscription workflow #64913

workflows: Add a simple pull request subscription workflow #64913

Conversation

tstellar commented Aug 23, 2023 • edited Loading

github-actions bot commented Aug 23, 2023

tstellar commented Aug 23, 2023

joker-eph commented Aug 23, 2023

MaskRay commented Aug 23, 2023

tstellar commented Aug 23, 2023

tstellar commented Aug 23, 2023

joker-eph commented Aug 23, 2023 • edited Loading

tstellar commented Aug 23, 2023

joker-eph commented Aug 24, 2023

tstellar commented Aug 24, 2023

asl commented Aug 25, 2023

smeenai commented Aug 28, 2023

boomanaiden154 commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstellar commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joker-eph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay commented Sep 8, 2023

MaskRay commented Sep 8, 2023

tru left a comment

Choose a reason for hiding this comment

jrtc27 commented Sep 8, 2023

tstellar commented Aug 23, 2023 •

edited

Loading

joker-eph commented Aug 23, 2023 •

edited

Loading

MaskRay Sep 8, 2023 •

edited

Loading