Skip to content

[GitHub][workflows] Add buildbot information comment to first merged PR from a new contributor #78292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 31, 2024

Conversation

DavidSpickett
Copy link
Collaborator

@DavidSpickett DavidSpickett commented Jan 16, 2024

This change adds a comment to the first PR from a new contributor that is merged, which tells them what to expect post merge from the build bots.

How they will be notified, where to ask questions, that you're more likely to be reverted than in other projects, etc. The information overlaps with, and links to https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr. So that users who simply read the email are still aware, and know where to follow up if they do get reports.

To do this, I have added a hidden HTML comment to the new contributor greeting comment. This workflow will look for that to tell if the author of the PR was a new contributor at the time they opened the merge. It has to be done this way because as soon as the PR is merged, they are by GitHub's definition no longer a new contributor and I suspect that their author association will be "contributor" instead.

I cannot 100% confirm that without a whole lot of effort and probably breaking GitHub's terms of service, but it's fairly cheap to work around anyway. It seems rare / almost impossible to reopen a PR in llvm at least, but in case it does happen the buildbot info comment has its own hidden HTML comment. If we find this we will not post another copy of the same information.

…PR from a new contributor

This change adds a comment to the first PR from a new contributor that is merged,
which tells them what to expect post merge from the build bots.

How they will be notified, where to ask questions, that you're more likely
to be reverted than in other projects, etc.

To do this, I have added a hidden HTML comment to the new contributor greeting comment.
This workflow will look for that to tell if the author of the PR was
a new contributor at the time they opened the merge.

It has to be done this way because as soon as the PR is merged, they are
by GitHub's definition no longer a new contributor and I suspect that
their author assocication will be "contributor" instead.

I cannot 100% confirm that without a whole lot of effort and probably breaking
GitHub's terms of service, but it's fairly cheap to work around anyway.

It seems rare / almost impossible to reopen a PR in llvm at least, but
in case it does happen the buildbot info comment has its own hidden
HTML comment. If we find this we will not post another copy of the
same information.

An example PR can be found here: #84
(exact text content subject to change)
@llvmbot
Copy link
Member

llvmbot commented Jan 16, 2024

@llvm/pr-subscribers-github-workflow

Author: David Spickett (DavidSpickett)

Changes

This change adds a comment to the first PR from a new contributor that is merged, which tells them what to expect post merge from the build bots.

How they will be notified, where to ask questions, that you're more likely to be reverted than in other projects, etc.

To do this, I have added a hidden HTML comment to the new contributor greeting comment. This workflow will look for that to tell if the author of the PR was a new contributor at the time they opened the merge.

It has to be done this way because as soon as the PR is merged, they are by GitHub's definition no longer a new contributor and I suspect that their author assocication will be "contributor" instead.

I cannot 100% confirm that without a whole lot of effort and probably breaking GitHub's terms of service, but it's fairly cheap to work around anyway.

It seems rare / almost impossible to reopen a PR in llvm at least, but in case it does happen the buildbot info comment has its own hidden HTML comment. If we find this we will not post another copy of the same information.

An example PR can be found here: DavidSpickett#84 (exact text content subject to change)


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

2 Files Affected:

  • (added) .github/workflows/merged-prs.yml (+36)
  • (modified) llvm/utils/git/github-automation.py (+71)
diff --git a/.github/workflows/merged-prs.yml b/.github/workflows/merged-prs.yml
new file mode 100644
index 00000000000000..1b1503610dac13
--- /dev/null
+++ b/.github/workflows/merged-prs.yml
@@ -0,0 +1,36 @@
+name: "Add buildbot information to first PRs from new contributors"
+
+permissions:
+  contents: read
+
+on:
+  # It's safe to use pull_request_target here, because we aren't checking out
+  # code from the pull request branch.
+  # See https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
+  pull_request_target:
+    types:
+      - closed
+
+jobs:
+  buildbot_comment:
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+    if: >-
+      (github.repository == 'llvm/llvm-project') &&
+      (github.event.pull_request.merged == true)
+    steps:
+      - name: Setup Automation Script
+        run: |
+          curl -O -L --fail https://raw.githubusercontent.com/"$GITHUB_REPOSITORY"/main/llvm/utils/git/github-automation.py
+          curl -O -L --fail https://raw.githubusercontent.com/"$GITHUB_REPOSITORY"/main/llvm/utils/git/requirements.txt
+          chmod a+x github-automation.py
+          pip install -r requirements.txt
+
+      - name: Add Buildbot information comment
+        run: |
+          ./github-automation.py \
+            --token '${{ secrets.GITHUB_TOKEN }}' \
+            pr-buildbot-information \
+            --issue-number "${{ github.event.pull_request.number }}" \
+            --author "${{ github.event.pull_request.user.login }}"
diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index f78d91059ecd36..55659679536f48 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -208,6 +208,8 @@ def _get_curent_team(self) -> Optional[github.Team.Team]:
 
 
 class PRGreeter:
+    COMMENT_TAG = "<!--LLVM NEW CONTRIBUTOR COMMENT-->\n"
+
     def __init__(self, token: str, repo: str, pr_number: int):
         repo = github.Github(token).get_repo(repo)
         self.pr = repo.get_issue(pr_number).as_pull_request()
@@ -217,7 +219,9 @@ def run(self) -> bool:
         # by a user new to LLVM and/or GitHub itself.
 
         # This text is using Markdown formatting.
+
         comment = f"""\
+{PRGreeter.COMMENT_TAG}
 Thank you for submitting a Pull Request (PR) to the LLVM Project!
 
 This PR will be automatically labeled and the relevant teams will be
@@ -240,6 +244,64 @@ def run(self) -> bool:
         return True
 
 
+class PRBuildbotInformation:
+    COMMENT_TAG = "<!--LLVM BUILDBOT INFORMATION COMMENT-->\n"
+
+    def __init__(self, token: str, repo: str, pr_number: int, author: str):
+        repo = github.Github(token).get_repo(repo)
+        self.pr = repo.get_issue(pr_number).as_pull_request()
+        self.author = author
+
+    def should_comment(self) -> bool:
+        # As soon as a new contributor has a PR merged, they are no longer a new contributor.
+        # We can tell that they were a new contributor previously because we would have
+        # added a new contributor greeting comment when they opened the PR.
+        found_greeting = False
+        for comment in self.pr.as_issue().get_comments():
+            if PRGreeter.COMMENT_TAG in comment.body:
+                found_greeting = True
+            elif PRBuildbotInformation.COMMENT_TAG in comment.body:
+                # When an issue is reopened, then closed as merged again, we should not
+                # add a second comment. This event will be rare in practice as it seems
+                # like it's only possible when the main branch is still at the exact
+                # revision that the PR was merged on to, beyond that it's closed forever.
+                return False
+        return found_greeting
+
+    def run(self) -> bool:
+        if not self.should_comment():
+            return
+
+        # This text is using Markdown formatting.
+        comment = f"""\
+{PRBuildbotInformation.COMMENT_TAG}
+@{self.author} Congratulations on having your first Pull Request (PR) merged into the LLVM Project!
+
+Your changes will be combined with recent changes from other authors, then tested
+by our [build bots](https://lab.llvm.org/buildbot/#/console).
+
+If there is a problem with the build, all the change authors will receive an email
+describing the problem. Please check whether the problem has been caused by your
+change, as the change set may include many authors. If the problem affects many
+configurations, you may get many emails for the same problem.
+
+If you are using a GitHub `noreply` email address, you will not receive these emails.
+Instead, someone will comment on this PR to inform you of the issue.
+
+If you do not receive any reports of problems, no action is required from you.
+Your changes are working as expected, well done!
+
+If your change causes an ongoing issue, it may be reverted. This is a [normal part of LLVM development](https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy)
+and is not a comment on yourself as an author.The revert commit (or a comment on this PR)
+should explain why it was reverted, how to fix your changes and merge them again.
+
+If you are unsure how to fix a problem, you can send questions in a reply
+to the notification email, add a comment to this PR, or ask on [Discord](https://discord.com/invite/xS7Z362).
+"""
+        self.pr.as_issue().create_comment(comment)
+        return True
+
+
 def setup_llvmbot_git(git_dir="."):
     """
     Configure the git repo in `git_dir` with the llvmbot account so
@@ -698,6 +760,10 @@ def execute_command(self) -> bool:
 pr_greeter_parser = subparsers.add_parser("pr-greeter")
 pr_greeter_parser.add_argument("--issue-number", type=int, required=True)
 
+pr_buildbot_information_parser = subparsers.add_parser("pr-buildbot-information")
+pr_buildbot_information_parser.add_argument("--issue-number", type=int, required=True)
+pr_buildbot_information_parser.add_argument("--author", type=str, required=True)
+
 release_workflow_parser = subparsers.add_parser("release-workflow")
 release_workflow_parser.add_argument(
     "--llvm-project-dir",
@@ -751,6 +817,11 @@ def execute_command(self) -> bool:
 elif args.command == "pr-greeter":
     pr_greeter = PRGreeter(args.token, args.repo, args.issue_number)
     pr_greeter.run()
+elif args.command == "pr-buildbot-information":
+    pr_buildbot_information = PRBuildbotInformation(
+        args.token, args.repo, args.issue_number, args.author
+    )
+    pr_buildbot_information.run()
 elif args.command == "release-workflow":
     release_workflow = ReleaseWorkflow(
         args.token,

@DavidSpickett
Copy link
Collaborator Author

Important to note that this comment will notify all users, there's no way to avoid that. I hope that by @ ing the author, it may be more friendly to email filters.

@DavidSpickett
Copy link
Collaborator Author

@DavidSpickett DavidSpickett requested review from tstellar and tru January 16, 2024 14:45
Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor comments. Thanks for working on this!

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DavidSpickett
Copy link
Collaborator Author

Thanks for reviewing! I'm going to wait a few days for any feedback from the RFC before landing.

@asb
Copy link
Contributor

asb commented Jan 21, 2024

This is a great idea - thanks! A few thoughts:

  • Are we certain that buildbot emails do go to the patch author and not just the committer? CC @gkistanova.
  • "If you are unsure how to fix a problem, you can send questions in a reply to the notification email". If your change is the only one causing a failure, then the only person to reply to is gkistanova (I think [email protected] just bounces). Otherwise, you're replying to other patch authors in the change list. Perhaps directing people to the PR or to discord would be better?
  • We do seem to have some buildbots that have spurious failures every now and again. I think it would be good to warn of that possibility. e.g. "On occasion buildbots will encounter a spurious failure (i.e. failing for a reason other than your change). If after reviewing the details of the failure you suspect this is the case, you may wish to check if there were similar failures in recent builds on that bot or wait to see if the next scheduled build fails." Maybe that's getting a bit lengthy though...

@DavidSpickett
Copy link
Collaborator Author

Maybe that's getting a bit lengthy though...

Given we have so many "but this", "sometimes that" I'm wondering if it's not better for me to update one of the documentation pages, then link to that. Then we have more room to include them. Not sure which yet though.

@DavidSpickett
Copy link
Collaborator Author

"If you are unsure how to fix a problem, you can send questions in a reply to the notification email". If your change is the only one causing a failure, then the only person to reply to is gkistanova (I think [email protected] just bounces). Otherwise, you're replying to other patch authors in the change list. Perhaps directing people to the PR or to discord would be better?

Yeah, not mentioning the email reply is probably better. The reviewers of the PR are more likely to have context on why it failed (I would hope), and they can sort it out from there.

@RalfJung
Copy link
Contributor

We do seem to have some buildbots that have spurious failures every now and again. I think it would be good to warn of that possibility. e.g. "On occasion buildbots will encounter a spurious failure (i.e. failing for a reason other than your change).

So far, I think I got buildbot failures after 100% of my PRs that landed here (which is 5). My PRs were all changing the LangRef and nothing else, so none of them was a real failure caused by my PR.

So I wouldn't say this is a rare occurrence, it seems to be the common case. My conclusion is that I should just entirely ignore these emails... that's probably not right either though, but I am not sure how I could even figure out whether I need to take any action.

@DavidSpickett
Copy link
Collaborator Author

Thanks for your feedback, this is exactly what I was looking for!

So I wouldn't say this is a rare occurrence, it seems to be the common case.

Yeah my bias comes in here seeing builds go by all the time that do work properly, I forget the important bit is the builds the specific committer sees. So your take away isn't wrong at all. Unfortunate, but not a failure on the part of committers, instead a sign that the system should be improved.

So I wouldn't say this is a rare occurrence, it seems to be the common case. My conclusion is that I should just entirely ignore these emails... that's probably not right either though, but I am not sure how I could even figure out whether I need to take any action.

We're asking a lot of folks to understand a whole other system, and explaining how to diagnose problems generally is too much to include here and something I wouldn't want folks to think they've got to spend too much time on.

Your feedback certainly backs up Alex's suggestions so I will add those now.

  this may not happen.
* Remove replying to emails because again, there's some situations
  where that won't be helpful.
@DavidSpickett
Copy link
Collaborator Author

I've updated the text with the feedback so far.

I think it could go on https://llvm.org/docs/GitHub.html instead as "what happens after merge" so going to look at that next.

@DavidSpickett
Copy link
Collaborator Author

@asb I've updated the text to link to https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr, but there's still a considerable overlap as you suggested.

My intent is that the text in the comment warns you about the initial messages you might get, then tells you, if you need to follow up on them, here's the link to a description of all that.

@asb
Copy link
Contributor

asb commented Jan 30, 2024

New text LGTM, but it looks like some of the newly added lines aren't line-wrapped while the existing lines are.

@DavidSpickett
Copy link
Collaborator Author

DavidSpickett commented Jan 30, 2024

Yeah so that's an annoying thing I've had to work around. Unlike RST the lines don't flow automatically regardless of the newlines. So I've tried to lay it out so that the paragraphs aren't too weird looking, but the text is still readable in an editor.

Also when you have a [link](giant URL...............................goes here) that renders into a single word link. So also tried to write it so we'd end up with a full line there. Instead of:

Some text with a <link>.
Next sentence ends up here which looks strange given it's supposed to be part of the same paragraph

@DavidSpickett
Copy link
Collaborator Author

Added a comment to explain that oddity.

Copy link
Contributor

@asb asb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd be OK with just having the comment in unwrapped form so that it's stored in the preferred form to be submitted as a GitHub comment, but the trade-off you make seems reasonable too.

@DavidSpickett DavidSpickett merged commit 44ba4c7 into llvm:main Jan 31, 2024
@DavidSpickett DavidSpickett deleted the new-user-merge branch January 31, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants