Page MenuHomePhabricator

github: Automatically create backport requests for bugs referenced in commit messages
Needs RevisionPublic

Authored by tstellar on Feb 10 2022, 12:10 AM.

Details

Reviewers
kwk
Summary

This adds a new action that will automatically create a backport request
for the release branch any time a bug is automatically closed by
having a reference to it (e.g. Fixes #12345) in a commit message.

Diff Detail

Event Timeline

tstellar requested review of this revision.Feb 10 2022, 12:10 AM
tstellar created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2022, 12:10 AM
kwk requested changes to this revision.Feb 22 2022, 2:40 PM
kwk added inline comments.
.github/workflows/issue-release-workflow.yml
18

I forgot to mention this but it's tricky with issue comments. An issue comment can also be a PR comment and you don't want your workflow to run then I bet. That's why you need to exclude PR comments. I know we don't have PRs yet but who knows. Here you can read on how to exclude PR comments from the game: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment-on-issues-only-or-pull-requests-only .

58

While this may be okay for now I suggest to make this selection explicit instead of indirectly implicit. The next time you add or remove an event type in the on section above (e.g. add labeled as an event type), the logic here breaks. I suggest to have this formulated out. Here's a list of all possible activity types: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issues . You certainly limit the scope in the on section already but the moment you widen this, you have to check this place again. And that should be avoided IMHO.

92

Same here, please make this explicit.

llvm/utils/git/github-automation.py
20

I suggest to use a query with placeholders and change this to:

def run_graphql_query(query: str, variables: dict, token: str) -> dict:

Also, please add a documentation since this will soon become a commonly called function I guess.

21–23

Maybe this is better depending on if you plan to use node IDs:

headers = {
    'Authorization' : 'bearer {}'.format(token),
    # See
    # https://github.blog/2021-11-16-graphql-global-id-migration-update/
    'X-Github-Next-Global-ID': '1'
}
26

Change to

json = {"query": query, "variables": variables}
277–308

I suggest to pass in a clean query so you can always copy'n'paste the query from here to execute it for testing purposes on the graphql explorer. The variables help with this. Here's a patch:

diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index 62c9886b52f0..b0fa1b3c126b 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -17,13 +17,16 @@ import requests
 import sys
 from typing import *
 
-def run_graphql_query(query: str, token: str) -> dict:
+def run_graphql_query(query: str, token: str, variables: dict) -> dict:
     headers = {
         'Authorization' : 'bearer {}'.format(token),
+        # See
+        # https://github.blog/2021-11-16-graphql-global-id-migration-update/
+        'X-Github-Next-Global-ID': '1'
     }
     request = requests.post(
         url = 'https://api.github.com/graphql',
-        json = {"query" : query },
+        json = {"query" : query , "variables": variables},
         headers = headers)
 
     if request.status_code == 200:
@@ -275,9 +278,9 @@ def create_cherry_pick_request_from_closed_issue(issue:int, repo_name:str, token
     [owner, name] = repo_name.split('/')
 
     query = """
-        query {
-            repository(owner: """ f'"{owner}"'', name: 'f'"{name}"'""") {
-                issue(number: """f'{issue}'""") {
+        query($owner: String!, $name: String!, $issue_number: Int!){
+            repository(owner: $owner, name: $name) {
+                issue(number: $issue_number) {
                     title
                     timelineItems (itemTypes: [CLOSED_EVENT], last: 1) {
                         edges {
@@ -306,8 +309,13 @@ def create_cherry_pick_request_from_closed_issue(issue:int, repo_name:str, token
                 }
             }
         }"""
+    variables = {
+        "owner": owner,
+        "name": name,
+        "issue_number": issue,
+    }
 
-    data = run_graphql_query(query, token)
+    data = run_graphql_query(query, variables, token)
     title = data['repository']['issue']['title']
     event = data['repository']['issue']['timelineItems']['edges']
     authors = []
@@ -328,7 +336,8 @@ def create_cherry_pick_request_from_closed_issue(issue:int, repo_name:str, token
             milestone = m
             break
 
-    message = '{} What do you think about backporting {}?\n\n/cherry-pick {}'.format(' '.join(['@' + login for login in assignees]), 'this' if len(commits) == 1 else 'these', ' '.join(commits))
+    annotated_assignees = ' '.join(['@' + login for login in assignees])
+    message = '{} What do you think about backporting {}?\n\n/cherry-pick {}'.format(annotated_assignees, 'this commit' if len(commits) == 1 else 'these commits', ' '.join(commits))
     gh_issue = repo.create_issue(title='Cherry-pick fixes for #{}: {}'.format(issue, title),
                                  assignees=assignees,
                                  milestone=milestone,
282

This is cool. It looks like you've taken into account that an issue could be closed and reopened. But I wonder if it is enough to just focus on the last close event. What if you close an issue with a commit but missed out and edge case and later create a fixup commit. This will not be covered here but afterall we're humans and need to judge. This is just a nudge in the right direction I guess.

331

Change:

this to this commit and
these to these commits.

You also might want to make the commits a set just in case they occur more than once.

332–335

You might want to label this issue in some way.

361

commits -> commit(s)

This revision now requires changes to proceed.Feb 22 2022, 2:40 PM