github-automation: Use a single comment for team mentions on pull requests #66037

tstellar · 2023-09-12T02:09:24Z

This will reduce the number of notifications created when a pull request label is added. Each team will only get a notification when their team's label is added and not when other teams' labels are added.

llvmbot · 2023-09-12T02:10:38Z

@llvm/pr-subscribers-github-workflow

Changes

…uests

This will reduce the number of notifications created when a pull request label is added. Each team will only get a notification when their team's label is added and not when other teams' labels are added.

Full diff: https://github.com/llvm/llvm-project/pull/66037.diff

2 Files Affected:

(modified) .github/workflows/pr-subscriber.yml (+8)
(modified) llvm/utils/git/github-automation.py (+19-2)

diff --git a/.github/workflows/pr-subscriber.yml b/.github/workflows/pr-subscriber.yml
index 3b18c8b35e97d1a..14e6cb33c57f9f6 100644
--- a/.github/workflows/pr-subscriber.yml
+++ b/.github/workflows/pr-subscriber.yml
@@ -9,6 +9,14 @@ on:
 permissions:
   contents: read
 
+concurrency:
+  # Ideally, we would use the PR number in the concurrency group, but we don't
+  # have access to it here.  We need to ensure only one job is running for
+  # each PR at a time, because there is a potentaill race condition when
+  # updating the issue comment.
+  group: "PR Subscriber"
+  cancel-in-progress: false
+
 jobs:
   auto-subscribe:
     runs-on: ubuntu-latest
diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index 7df20ea8457155b..a7b88ed383a1238 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -83,6 +83,15 @@ def __init__(self, token: str, repo: str, pr_number: int, label_name: str):
         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()
+        self.COMMENT_TAG = "\n"
+
+    def get_summary_comment(self) -> github.IssueComment.IssueComment:
+        for comment in self.pr.as_issue().get_comments():
+            if not self.COMMENT_TAG in comment.body:
+                continue
+            return comment
+        return None
+
 
     def run(self) -> bool:
         patch = None
@@ -119,10 +128,12 @@ def run(self) -> bool:
             patch_link = f"\nPatch is {human_readable_size(len(patch))}, truncated to {human_readable_size(DIFF_LIMIT)} below, full version: {self.pr.diff_url}\n"
             diff_stats = diff_stats[0:DIFF_LIMIT] + "...\n\n"
         diff_stats += "

"

   team_mention = "@llvm/{}".format(team.slug)

   body = self.pr.body
   comment = (

```
       "@llvm/{}".format(team.slug)
```

```
       self.COMMENT_TAG
```

       + team_mention
       + "\n\n<details>\n"
       + f"<summary>Changes</summary>\n\n"
       + f"{body}\n--\n"

@@ -132,7 +143,13 @@ def run(self) -> bool:
+ "

"
)

   self.pr.as_issue().create_comment(comment)

   summary_comment = self.get_summary_comment()

```
   if not summary_comment:
```

       self.pr.as_issue().create_comment(comment)

   elif team_mention + "\n" in summary_comment.body:

       print("Team {} already mentioned.".format(team.slug))

```
   else:
```

       summary_comment.edit(summary_comment.body.replace(self.COMMENT_TAG, self.COMMENT_TAG + team_mention + "\n"))
   return True

def _get_curent_team(self) -> Optional[github.Team.Team]:

joker-eph · 2023-09-12T03:26:13Z

Please fix the title wrapping
(it's an annoying GitHub things, I don't think we can control it, maybe another action to write?)

.github/workflows/pr-subscriber.yml

llvm/utils/git/github-automation.py

cor3ntin

LGTM modulo a minor suggestion

llvm/utils/git/github-automation.py

tru

LGTM, except the black formatter failing.

llvm/utils/git/github-automation.py

cor3ntin · 2023-09-12T17:23:46Z

Could we replace @ in the description by something else so that people don't get extra mentions?
all the bugzilla bugs have a CC fields that ping everyone in the issue, putting that in the description is noisy for some folks

Endilll · 2023-09-12T17:25:54Z

all the bugzilla bugs have a CC fields that ping everyone in the issue

Quite often it's not helpful. On Clang side it's not helpful at all.

cor3ntin · 2023-09-12T17:26:27Z

Test @Endilll

tstellar · 2023-09-12T17:28:47Z

Could we replace @ in the description by something else so that people don't get extra mentions? all the bugzilla bugs have a CC fields that ping everyone in the issue, putting that in the description is noisy for some folks

This is the only way that I know of to subscribe someone to a pull request. However, I've tested this out and people only get notifications for the first edit that mentions a team they are on. So if you have already been mentioned and another team gets mentioned, you won't see a notification for that.

cor3ntin · 2023-09-12T17:30:53Z

@tstellar the issue is that people get mentioned twice because of the content of details, not because of the intentional @llvm/ mention. I'm working on fixing that

cor3ntin · 2023-09-12T17:32:52Z

llvm/utils/git/github-automation.py

        diff_stats += "</pre>"
+        team_mention = "@llvm/{}".format(team.slug)

        body = self.pr.body


Suggested change

body = self.pr.body

body = self.pr.body.replace('@', '@')

From github/markup#1168

As discussed on Discord, I don't think this is an issue for pull requests.

llvmbot · 2023-09-12T18:37:44Z

@llvm/pr-subscribers-github-workflow

Changes

This will reduce the number of notifications created when a pull request label is added. Each team will only get a notification when their team's label is added and not when other teams' labels are added. -- Full diff: https://github.com//pull/66037.diff

2 Files Affected:

(modified) .github/workflows/pr-subscriber.yml (+8)
(modified) llvm/utils/git/github-automation.py (+33-12)

diff --git a/.github/workflows/pr-subscriber.yml b/.github/workflows/pr-subscriber.yml
index 3b18c8b35e97d1a..60c884cb172c2a2 100644
--- a/.github/workflows/pr-subscriber.yml
+++ b/.github/workflows/pr-subscriber.yml
@@ -9,6 +9,14 @@ on:
 permissions:
   contents: read
 
+concurrency:
+  # Ideally, we would use the PR number in the concurrency group, but we don't
+  # have access to it here.  We need to ensure only one job is running for
+  # each PR at a time, because there is a potential race condition when
+  # updating the issue comment.
+  group: "PR Subscriber"
+  cancel-in-progress: false
+
 jobs:
   auto-subscribe:
     runs-on: ubuntu-latest
diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index c4c4848fbc5f8bc..cbad9bd829beffb 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -10,6 +10,7 @@
 
 import argparse
 from git import Repo  # type: ignore
+import html
 import github
 import os
 import re
@@ -95,6 +96,13 @@ def __init__(self, token: str, repo: str, pr_number: int, label_name: str):
         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()
+        self.COMMENT_TAG = "<!--LLVM PR SUMMARY COMMENT-->\n"
+
+    def get_summary_comment(self) -> github.IssueComment.IssueComment:
+        for comment in self.pr.as_issue().get_comments():
+            if self.COMMENT_TAG in comment.body:
+                return comment
+        return None
 
     def run(self) -> bool:
         patch = None
@@ -129,22 +137,35 @@ def run(self) -> bool:
         patch_link = f"Full diff: {self.pr.diff_url}\n"
         if len(patch) > DIFF_LIMIT:
             patch_link = f"\nPatch is {human_readable_size(len(patch))}, truncated to {human_readable_size(DIFF_LIMIT)} below, full version: {self.pr.diff_url}\n"
-            diff_stats = diff_stats[0:DIFF_LIMIT] + "...\n<truncated>\n"
+            diff_stats = html.escape(diff_stats[0:DIFF_LIMIT]) + "...\n<truncated>\n"
         diff_stats += "</pre>"
+        team_mention = "@llvm/{}".format(team.slug)
 
         body = self.pr.body
-        comment = (
-            "@llvm/{}".format(team.slug)
-            + "\n\n<details>\n"
-            + f"<summary>Changes</summary>\n\n"
-            + f"{body}\n--\n"
-            + patch_link
-            + "\n"
-            + f"{diff_stats}\n\n"
-            + "</details>"
-        )
+        comment = f"""
+{self.COMMENT_TAG}
+{team_mention}
+            
+<details>
+<summary>Changes</summary>
+{body}
+--
+{patch_link}
+{diff_stats}
+</details>
+"""
 
-        self.pr.as_issue().create_comment(comment)
+        summary_comment = self.get_summary_comment()
+        if not summary_comment:
+            self.pr.as_issue().create_comment(comment)
+        elif team_mention + "\n" in summary_comment.body:
+            print("Team {} already mentioned.".format(team.slug))
+        else:
+            summary_comment.edit(
+                summary_comment.body.replace(
+                    self.COMMENT_TAG, self.COMMENT_TAG + team_mention + "\n"
+                )
+            )
         return True
 
     def _get_curent_team(self) -> Optional[github.Team.Team]:

tstellar · 2023-09-12T19:07:16Z

@cor3ntin Thanks for the feedback, this looks really good now. I'll push this later today if there are no other comments.

cor3ntin · 2023-09-12T19:10:42Z

Thanks, lgtm!

…uests This will reduce the number of notifications created when a pull request label is added. Each team will only get a notification when their team's label is added and not when other teams' labels are added.

…uests (llvm#66037) This will reduce the number of notifications created when a pull request label is added. Each team will only get a notification when their team's label is added and not when other teams' labels are added.

tstellar requested a review from a team as a code owner September 12, 2023 02:09

llvmbot added the github:workflow label Sep 12, 2023

tstellar requested a review from jh7370 September 12, 2023 02:10

joker-eph reviewed Sep 12, 2023

View reviewed changes

.github/workflows/pr-subscriber.yml Outdated Show resolved Hide resolved

joker-eph reviewed Sep 12, 2023

View reviewed changes

llvm/utils/git/github-automation.py Outdated Show resolved Hide resolved

joker-eph approved these changes Sep 12, 2023

View reviewed changes

tstellar changed the title ~~github-automation: Use a single comment for team mentions on pull req…~~ github-automation: Use a single comment for team mentions on pull requests Sep 12, 2023

tstellar force-pushed the single-mention-comment branch from 7e08d10 to e483769 Compare September 12, 2023 06:22

cor3ntin approved these changes Sep 12, 2023

View reviewed changes

llvm/utils/git/github-automation.py Outdated Show resolved Hide resolved

tru approved these changes Sep 12, 2023

View reviewed changes

cor3ntin approved these changes Sep 12, 2023

View reviewed changes

cor3ntin reviewed Sep 12, 2023

View reviewed changes

llvm/utils/git/github-automation.py Outdated Show resolved Hide resolved

cor3ntin reviewed Sep 12, 2023

View reviewed changes

llvm deleted a comment from llvmbot Sep 12, 2023

github-automation: Use a single comment for team mentions on pull req…

512a646

…uests This will reduce the number of notifications created when a pull request label is added. Each team will only get a notification when their team's label is added and not when other teams' labels are added.

tstellar force-pushed the single-mention-comment branch from b6a7841 to 512a646 Compare September 12, 2023 20:12

tstellar merged commit 64751ea into llvm:main Sep 12, 2023

vzakhari mentioned this pull request Sep 12, 2023

internap proc trampolines #66156

Closed

joker-eph mentioned this pull request Sep 14, 2023

Improve the pull-request subcription notification format by adding the description and files statistics #65828

Merged

	body = self.pr.body
	body = self.pr.body.replace('@', '@<!-- -->')

github-automation: Use a single comment for team mentions on pull requests #66037

github-automation: Use a single comment for team mentions on pull requests #66037

Uh oh!

Conversation

tstellar commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 12, 2023

This will reduce the number of notifications created when a pull request label is added. Each team will only get a notification when their team's label is added and not when other teams' labels are added.

Uh oh!

joker-eph commented Sep 12, 2023

Uh oh!

Uh oh!

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tru left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cor3ntin commented Sep 12, 2023

Uh oh!

Endilll commented Sep 12, 2023

Uh oh!

cor3ntin commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstellar commented Sep 12, 2023

Uh oh!

cor3ntin commented Sep 12, 2023

Uh oh!

cor3ntin Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

tstellar Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Sep 12, 2023

Uh oh!

tstellar commented Sep 12, 2023

Uh oh!

cor3ntin commented Sep 12, 2023

Uh oh!

Uh oh!

tstellar commented Sep 12, 2023 •

edited

Loading

tru left a comment •

edited

Loading

cor3ntin commented Sep 12, 2023 •

edited

Loading