This is an archive of the discontinued LLVM Phabricator instance.

github: Automatically assign reviewers for backport requests
ClosedPublic

Authored by tstellar on May 25 2022, 2:15 PM.

Details

Summary

When there is a backport request, the GitHub Action that handles the
backport will now automatically assign the issue to the user(s) who
approved the commit in Phabricator and create an issue comment asking
them to review the request.

Diff Detail

Event Timeline

tstellar created this revision.May 25 2022, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 2:15 PM
tstellar requested review of this revision.May 25 2022, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 2:15 PM
kwk requested changes to this revision.Jul 6 2022, 9:32 AM
kwk added a subscriber: kwk.

Can you please add links to the phabricator HTTP API documentation that you're using?

llvm/utils/git/github-automation.py
62
245

When I use capital L I get: NameError: name 'List' is not defined. Did you mean: 'list'?

267

What if the list of reviewers is empty? Don't you want to skip that call to add_to_assignees?

285

Do you still want to continue when the exception was caught?

392
This revision now requires changes to proceed.Jul 6 2022, 9:32 AM
thieta added inline comments.Jul 6 2022, 9:37 AM
llvm/utils/git/github-automation.py
245

List is a typing hint. You need to import it from the typing module.

tstellar added inline comments.Jul 7 2022, 5:44 PM
llvm/utils/git/github-automation.py
245

When I use list[str], I get:

TypeError: 'type' object is not subscriptable

tstellar updated this revision to Diff 443137.Jul 7 2022, 9:57 PM
tstellar marked 5 inline comments as done.

Re-wrote the patch so that the pull request is assigned to the reviewers rather
than the issue. This seems to make more sense since the reviewers will be reviewing
the code.

lkail added a subscriber: lkail.Jul 7 2022, 9:57 PM
tstellar added inline comments.Jul 7 2022, 9:58 PM
llvm/utils/git/github-automation.py
285

Yes, we want to continue, because assigning reviewers is optional.

thieta accepted this revision.Jul 8 2022, 1:43 AM

LGTM.

kwk added inline comments.Jul 11 2022, 3:53 AM
llvm/utils/git/github-automation.py
245

I think this is deprecated since Python 3.9 (https://docs.python.org/3/library/typing.html#typing.List). And this whole discussion rings a bell. @tstellar weren't the Github runners running an older version than what was required for list[str] to work?

tstellar added inline comments.Jul 11 2022, 9:18 AM
llvm/utils/git/github-automation.py
245

@kwk Yes, I believe we ran into the same issue on another patch. I see we use List one other place in the patch

kwk accepted this revision.Jul 26 2022, 1:06 AM

LGTM.

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

I tried to find limit in the documentation (see section "Paging and Limits") and it is always used like this:

{
   "cursor": {
       "limit": 100
   }
}

I wonder if this only works because of luck that there only is 1 result.

94

Are fields safe to access or is there any guarantee that a field exists in a result? Should fields be tested for existence before accessing them?

This revision is now accepted and ready to land.Jul 26 2022, 1:06 AM
tstellar added inline comments.Jul 26 2022, 1:19 AM
llvm/utils/git/github-automation.py
78

I think that's just the output json? If you put in an example query with the limit parameter, it shows the limit value being passed to curl simply as limit=1.

94

It looks like the fields are always there based on my testing.

This revision was landed with ongoing or failed builds.Jul 26 2022, 3:30 PM
This revision was automatically updated to reflect the committed changes.