[PATCH] D119412: github: Automatically create backport requests for bugs referenced in commit messages
Konrad Wilhelm Kleine via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 22 14:40:17 PST 2022
kwk requested changes to this revision.
kwk added inline comments.
This revision now requires changes to proceed.
================
Comment at: .github/workflows/issue-release-workflow.yml:18
on:
issue_comment:
types:
----------------
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 .
================
Comment at: .github/workflows/issue-release-workflow.yml:58
(github.repository == 'llvm/llvm-project') &&
+ github.event.action != 'closed' &&
!startswith(github.event.comment.body, '<!--IGNORE-->') &&
----------------
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.
================
Comment at: .github/workflows/issue-release-workflow.yml:92
(github.repository == 'llvm/llvm-project') &&
+ github.event.action != 'closed' &&
!startswith(github.event.comment.body, '<!--IGNORE-->') &&
----------------
Same here, please make this explicit.
================
Comment at: llvm/utils/git/github-automation.py:20
+def run_graphql_query(query: str, token: str) -> dict:
+ headers = {
----------------
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.
================
Comment at: llvm/utils/git/github-automation.py:21-23
+ headers = {
+ 'Authorization' : 'bearer {}'.format(token),
+ }
----------------
Maybe this is better depending on if you plan to use node IDs:
```lang=python
headers = {
'Authorization' : 'bearer {}'.format(token),
# See
# https://github.blog/2021-11-16-graphql-global-id-migration-update/
'X-Github-Next-Global-ID': '1'
}
```
================
Comment at: llvm/utils/git/github-automation.py:26
+ url = 'https://api.github.com/graphql',
+ json = {"query" : query },
+ headers = headers)
----------------
Change to
```lang=python
json = {"query": query, "variables": variables}
```
================
Comment at: llvm/utils/git/github-automation.py:277-308
+ query = """
+ query {
+ repository(owner: """ f'"{owner}"'', name: 'f'"{name}"'""") {
+ issue(number: """f'{issue}'""") {
+ title
+ timelineItems (itemTypes: [CLOSED_EVENT], last: 1) {
+ edges {
----------------
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:
```lang=diff
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,
```
================
Comment at: llvm/utils/git/github-automation.py:282
+ title
+ timelineItems (itemTypes: [CLOSED_EVENT], last: 1) {
+ edges {
----------------
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.
================
Comment at: llvm/utils/git/github-automation.py:331
+
+ 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))
+ gh_issue = repo.create_issue(title='Cherry-pick fixes for #{}: {}'.format(issue, title),
----------------
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.
================
Comment at: llvm/utils/git/github-automation.py:332-335
+ gh_issue = repo.create_issue(title='Cherry-pick fixes for #{}: {}'.format(issue, title),
+ assignees=assignees,
+ milestone=milestone,
+ body=message)
----------------
You might want to label this issue in some way.
================
Comment at: llvm/utils/git/github-automation.py:361
+request_cherry_pick_parser.add_argument('--issue-number', type=int,
+ help='Backport the commits that fixed ISSUE_NUMBER')
+
----------------
`commits` -> `commit(s)`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119412/new/
https://reviews.llvm.org/D119412
More information about the llvm-commits
mailing list