Skip to content
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

[GitHub] Add Greeting comment to new contributor's PRs with useful information #72249

Closed
wants to merge 1 commit into from

Conversation

DavidSpickett
Copy link
Collaborator

@DavidSpickett DavidSpickett commented Nov 14, 2023

This adds a new workflow that responds to PRs that are opened
by new contributors with a comment thanking the author for their contribution,
and provides answers to common problems (problem for now, could expand later).

According to my testing, and the docs here:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

opened will only trigger on the first opening, not a re-open.

I considered including this comment in the one the labeller adds,
but that means that it would be emailed to all subscribers not just
the author.

This comment is only left for authors new to the LLVM repo or to
GitHub itself. This is done by checking the value in:
https://docs.github.com/en/graphql/reference/enums#commentauthorassociation

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 14, 2023

@llvm/pr-subscribers-github-workflow

Author: David Spickett (DavidSpickett)

Changes

This adds a new workflow that responds to PRs that are opened with a comment thanking the author for their contribution, and provides answers to common problems (problem for now, could expand later).

According to my testing, and the docs here:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

opened will only trigger on the first opening, not a re-open.

I considered including this comment in the one the labeller adds, but that means that it would be emailed to all subscribers not just the author.

Though it's still possible that this workflow would end up commenting after another one, in which case subscribers would get a notification.


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

2 Files Affected:

  • (added) .github/workflows/pr-greeter.yml (+27)
  • (modified) llvm/utils/git/github-automation.py (+35)
diff --git a/.github/workflows/pr-greeter.yml b/.github/workflows/pr-greeter.yml
new file mode 100644
index 000000000000000..9bc7ed734ccd34e
--- /dev/null
+++ b/.github/workflows/pr-greeter.yml
@@ -0,0 +1,27 @@
+name: PR Greeter
+
+on:
+  pull_request_target:
+   types: [ opened ]
+
+permissions:
+  pull-requests: write
+
+jobs:
+  greet:
+    runs-on: ubuntu-latest
+    if: github.repository == 'llvm/llvm-project'
+    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: Greet Author
+        run: |
+          ./github-automation.py \
+            --token '${{ secrets.GITHUB_TOKEN }}' \
+            pr-greeter \
+            --issue-number "${{ github.event.pull_request.number }}"
diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index ad1878d41193920..be4962b8093ce9e 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -211,6 +211,35 @@ def _get_curent_team(self) -> Optional[github.Team.Team]:
         return None
 
 
+class PRGreeter:
+    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()
+
+    def run(self) -> bool:
+        # We assume that this is only called for a PR that has just been opened.
+
+        # This text is using Markdown formatting.
+        comment = f"""\
+Thank you for submitting a Pull Request (PR) to the LLVM Project!
+
+You can add reviewers by using the "Reviewers" section on this page.
+
+If this is not working for you, it's probably because you don't have write
+permissions for the repository. In which case you can instead tag reviewers by
+name in a comment by using `@` followed by their GitHub username.
+
+If you have received no comments on your PR for a week, you can request a review
+by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
+is once a week. Please remember that you are asking for valuable time from other developers.
+
+If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html).
+
+You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/)."""
+        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
@@ -655,6 +684,9 @@ def execute_command(self) -> bool:
 pr_subscriber_parser.add_argument("--label-name", type=str, required=True)
 pr_subscriber_parser.add_argument("--issue-number", type=int, required=True)
 
+pr_greeter_parser = subparsers.add_parser("pr-greeter")
+pr_greeter_parser.add_argument("--issue-number", type=int, required=True)
+
 release_workflow_parser = subparsers.add_parser("release-workflow")
 release_workflow_parser.add_argument(
     "--llvm-project-dir",
@@ -705,6 +737,9 @@ def execute_command(self) -> bool:
         args.token, args.repo, args.issue_number, args.label_name
     )
     pr_subscriber.run()
+elif args.command == "pr-greeter":
+    pr_greeter = PRGreeter(args.token, args.repo, args.issue_number)
+    pr_greeter.run()
 elif args.command == "release-workflow":
     release_workflow = ReleaseWorkflow(
         args.token,

@DavidSpickett
Copy link
Collaborator Author

Example PR: DavidSpickett#23

The mechanics are right I think, probably needs tuning so the right level of notifications are sent. I can look into figuring out if the author is new to the project, or the lower-tech version of that is a team you can join to opt out of these messages.

It could be done as part of the subscriber workflow, which already has a way to know if it has left a comment previously. That's one way to know what order all this will be done if that's required.

@nikic
Copy link
Contributor

nikic commented Nov 14, 2023

I don't want this kind of comment posted on every single PR (our PR notifications are already very spammy as it is), but if it were limited to pull_request.author_association == FIRST_TIME_CONTRIBUTOR, this may make sense.

.github/workflows/pr-greeter.yml Outdated Show resolved Hide resolved
# We assume that this is only called for a PR that has just been opened.

# This text is using Markdown formatting.
comment = f"""\
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this comment content. The question is do we want to add this to every pull request or do we want to only use it on certain pull requests, like from new contributors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds like folks would like it limited, and starting with only new contributors can't do any harm anyway, so I'll implement that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment content in proposed in the PR right now seems geared towards new (or the very occasional) contributor. Therefore, it would be best to only put this on PRs created by those type of contributors.
I guess that one heuristic to cover "very occasional" contributor could be to put this on every PR where the contributor hasn't contributed to LLVM in e.g. a year? Or the first PR for that contributor this year (which would result in frequent contributors seeing this once per year)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have seen some prebuilt actions that list all the PRs for a project and keep searching backwards until they see (or don't see) a PR from the same author. Though I wonder how that would work on LLVM's scale.

For now I've used a filter in the job YAML which is more brittle but will catch a decent number of cases.

…formation

This adds a new workflow that responds to PRs that are opened
by new contributors with a comment thanking the author for their contribution,
and provides answers to common problems (problem for now, could expand later).

According to my testing, and the docs here:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

`opened` will only trigger on the first opening, not a re-open.

I considered including this comment in the one the labeller adds,
but that means that it would be emailed to all subscribers not just
the author.

This comment is only left for authors new to the LLVM repo or to
GitHub itself. This is done by checking the value in:
https://docs.github.com/en/graphql/reference/enums#commentauthorassociation
@DavidSpickett DavidSpickett changed the title [GitHub] Add Greeting comment to new PRs with useful information [GitHub] Add Greeting comment to new contributor's PRs with useful information Nov 14, 2023
@DavidSpickett
Copy link
Collaborator Author

DavidSpickett commented Nov 14, 2023

I've tested this looking for the OWNER condition instead on my fork and it seems to work. FIRST_TIME_CONTRIBUTOR might imply FIRST_TIMER, but the docs don't say either way and being explicit is better anyway.

comment = f"""\
Thank you for submitting a Pull Request (PR) to the LLVM Project!

You can add reviewers by using the "Reviewers" section on this page.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about a sentence before this that explains that prs are automatically labeled and the respective teams for these labels will automatically be pinged. I would still keep the rest of the reviewer part, because the user might want to ping specific people for their involvement in that part of the code-base.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or might this be obvious by the time the action ran, because the labeling and team ping already happened?

Copy link
Collaborator Author

@DavidSpickett DavidSpickett Nov 14, 2023

Choose a reason for hiding this comment

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

A note is a good idea regardless, but as it stands, there's no explicit ordering of the jobs. This could finish before or after the labeller.

Maybe it would be better to include this workflow as the first job as the labeler (.github/workflows/new-prs.yml) and have the labeler depend on that job having run (or been skipped). Then we would always have greeting (or not) - labelling - subscribers.

And in theory, only the author gets the email with the greeting notification.

@DavidSpickett
Copy link
Collaborator Author

Maybe it would be better to include this workflow as the first job as the labeler (.github/workflows/new-prs.yml) and have the labeler depend on that job having run (or been skipped). Then we would always have greeting (or not) - labelling - subscribers.

I will work on this today so you all can at least see what it would look like. If it's relatively neat I think the guaranteed order would be nice to have.

@DavidSpickett
Copy link
Collaborator Author

It's sufficiently different it didn't make sense to do as a fixup here, so opened #72384 for the version where we always know what the job order is.

@DavidSpickett
Copy link
Collaborator Author

Closing this to make it clear which is my preferred version. Can always re-open this if I guessed wrong.

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.

None yet

6 participants