This is an archive of the discontinued LLVM Phabricator instance.

github: Add actions to automate part of the release workflow
ClosedPublic

Authored by tstellar on Jan 14 2022, 10:47 PM.

Details

Summary

This adds support for automatically cherry-picking and testing fixes for the
release branch using 'commands' in issue comments. The two supported commands are:

/cherry-pick <commit1> <commit2> ...

Which will backport and test commits from main. And also

/branch owner/repo/branch

Which will test commits from the given branch.

Diff Detail

Event Timeline

tstellar requested review of this revision.Jan 14 2022, 10:47 PM
tstellar created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2022, 10:47 PM
alexbatashev requested changes to this revision.Jan 17 2022, 2:17 AM
alexbatashev added a subscriber: alexbatashev.
alexbatashev added inline comments.
.github/workflows/issue-release-workflow.yml
14

Something like this should avoid interference with LLVM forks and their automation

43

Same as above

This revision now requires changes to proceed.Jan 17 2022, 2:17 AM
tstellar updated this revision to Diff 401897.Jan 21 2022, 1:25 AM

Restrict the action to just the llvm/llvm-project repo.

tstellar marked 2 inline comments as done.Jan 21 2022, 1:25 AM
alexbatashev accepted this revision.Jan 21 2022, 1:33 AM

LGTM now, thanks @tstellar

.github/workflows/issue-release-workflow.yml
43

Typo

This revision is now accepted and ready to land.Jan 21 2022, 1:33 AM
tstellar updated this revision to Diff 401902.Jan 21 2022, 1:40 AM

Fix typo.

tstellar marked an inline comment as done.Jan 21 2022, 1:41 AM
kwk requested changes to this revision.Jan 21 2022, 3:46 AM
kwk added a subscriber: kwk.

Again I feel picky and sorry. Due to documentation issues among other issues I have to reject this for the time being. There is literally no comment whatsoever. This would tremendously speed up the review speed because then you can more easily review what's a function is supposed to do and what it actually does.

.github/workflows/issue-release-workflow.yml
14

If this gets too long you might as well do this:

if: >-
    github.repository == 'llvm/llvm-project' &&
    startswith(github.event.comment.body, '/cherry-pick')

Your solution is not wrong.

25

I highly suggest to use a requirements.txt file instead. I suggest you create a requirements.txt.in with this content:

# Convert this file into a requirements.txt file by running:
#
# pip install pip-tools
# pip-compile -o requirements.txt  requirements.txt.in

PyGithub==1.55
GitPython==3.1.26

Then you convert this file locally into a requirements.txt file by running:

pip install pip-tools
pip-compile -o requirements.txt  requirements.txt.in

Find out more about requirements.txt here: https://pip.pypa.io/en/stable/reference/pip_install/#requirements-file-format

One of the added benefits is that you can more easily see when your code is affected by any CVEs and you can maintain direct and indirect dependencies separately. Also local execution of any python code can happen without looking at this nasty github workflow file.

Of course, version both files, requirements.txt.in and requirements.txt.

In this step things would then looks like this:

- pip install PyGithub GitPython
+ pip install -r requirements.txt
43

Same applies here for using multiline ifs if you want.

48

Why curl the file instead of cloning like before? If github-automation.py happens to need another file from the repo you'd have to remember to patch this place.

50

Another benefit of requirements.txt is that you have to maintain dependencies in one place only.

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

Please have a documentation for these functions.

47

Please describe what this class is supposed to do and use typed-python for return types and parameters. It tremendously will help newcomers and maintainers .

49

I suggest to keep this "private": self._args = args

59

I suggest to make this a property and the rest of the getters that do simple stuff as well:

@property
def issue_number(self) -> int:
    return self._args.issue_number
264

Please use the help='...' parameter to describe what each parameter is doing.

This revision now requires changes to proceed.Jan 21 2022, 3:46 AM
tstellar marked an inline comment as not done.Jan 21 2022, 9:43 PM

Thanks for the feedback. I will work on these suggestions.

.github/workflows/issue-release-workflow.yml
25

How important is it to have the version numbers in requirements.txt ? I'm concerned that the versions available in the github actions runner will change often and that will break this workflow if we have explicit versions.

48

I mainly did this because cloning is so slow. I agree cloning would be better. I will try to benchmark with a shallow clone to see how long it takes.

50

That would be nice. I considered putting this step into a reusable workflow, so it could be more easily shared, but I wasn't sure it was worth it for something so small.

tstellar updated this revision to Diff 402766.Jan 24 2022, 10:33 PM

Add some comments and a few other minor fixes

tstellar marked 8 inline comments as done.Jan 24 2022, 10:36 PM
tstellar added inline comments.
.github/workflows/issue-release-workflow.yml
48

I dropped this, the shallow checkout seems to be fast enough.

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

I removed self.args completely.

kwk added a comment.Jan 26 2022, 8:04 AM

Thank you @tstellar for accepting so many of my suggestions. I have a few more and then I will give it a full review again.

.github/workflows/issue-release-workflow.yml
25

The version is at least super important and helpful for local development and testing.

I wouldn't bother about the runner because pip is used to install these versions and the actions runner shouldn't have an impact on it. For example, this page lists the version of the library and one should be able to install any of them: https://pypi.org/project/PyGithub/#history .

Here's an example:

kkleine@work:~$ python3 -m venv $PWD/myenv
kkleine@work:~$ cd myenv/
kkleine@work:~/myenv$ source bin/activate
kkleine@work:~/myenv$ pip install PyGithub==1.45
Collecting PyGithub==1.45
  Downloading PyGithub-1.45.tar.gz (113 kB)
...
    Running setup.py install for PyGithub ... done
Successfully installed PyGithub-1.45 certifi-2021.10.8 charset-normalizer-2.0.10 deprecated-1.2.13 idna-3.3

If you want to be super careful, you might employ the idea of the virtual environment to not mess with system installed versions. In fact, I'd really do that! Afterall, these are all the steps needed in the step:

python3 -m venv $PWD/myenv
source myenv/source bin/activate

The only possible downside is that you'd have to source the activate script in all steps.

48

Thank you.

50

That would be nice. I considered putting this step into a reusable workflow, so it could be more easily shared, but I wasn't sure it was worth it for something so small.

I don't think you need a sharable workflow for this. Afterall it's just pip install -r requirements.txt. The point is, that the requirements file bundles everything.

Not sure if I mentioned this, but the requirements.txt file will be beneficial for local testing and if you have the versions in the file, you can be more certain, that github will do the same.

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

I suggest to change the interface from

def create_branch(self, commits:list):

to

def create_branch(self, commits:list[str]) -> bool:

It is possible to have types for nested elements, so here: list[str] but it seems like it is ignored when executed:

>>> foo([1,2,3])
1
2
3
>>> foo(["1", "2", "3"])
1
2
3

An editor on the other hand nicely presents the correct format to pass into this function:

149–150

I wonder what this sentence is supposed to mean? How is a branch associated with an issue number? By a comment? The description of this change doesn't indicate how this is done.

211–212

Why not return self.create_branch(args.split())?

219–220

Same here, why not return self.create_pull_request(owner, branch)?

tstellar updated this revision to Diff 403900.Jan 27 2022, 11:23 PM
tstellar marked 7 inline comments as done.

Address more review comments.

tstellar added inline comments.Jan 27 2022, 11:24 PM
.github/workflows/issue-release-workflow.yml
25

OK, thanks for the explanation. I've added the requirements.txt file.

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

I tried this, but python gave an error saying this wasn't supported.

tstellar marked 2 inline comments as done.Jan 27 2022, 11:25 PM
tstellar updated this revision to Diff 404227.Jan 28 2022, 9:48 PM
  • Added from typing import * for List[Str]
  • Reworded some of the error messages.
  • Allow commands on lines besides the first.
kwk updated this revision to Diff 404960.Feb 1 2022, 8:59 AM
  • More typed python and properties
kwk added a comment.Feb 1 2022, 9:09 AM

Looks good overall. Thank for your patience. Please make sure it still works after my patch ;/

.github/workflows/issue-release-workflow.yml
2

Please add a description in YAML comment here so one gets an immediate idea for whom this script is and what it does.

13

Good that you've fixed the OS version.

17

I assume that checking for the /cherry-pick anywhere in the comment body is part of the Allow commands on lines besides the first., right?

32

I never checked this but are you sure the comment body is properly escaped if it contains an apostrophe '? Does it make sense to do parse it here using something like the following? The benefit would be that you can easily test the extraction from the body with tests and that you could make a clean call to the final script.

printf '${{ github.event.comment.body }}' | ./llvm/utils/git/parse-comment-body.py --get-cherry-pick-commit
kwk accepted this revision.Feb 1 2022, 9:09 AM
This revision is now accepted and ready to land.Feb 1 2022, 9:09 AM
tstellar updated this revision to Diff 405097.Feb 1 2022, 2:04 PM
  • Add comments
  • Use environment variables to properly escape comments
tstellar marked 5 inline comments as done.Feb 1 2022, 2:06 PM
tstellar added inline comments.
.github/workflows/issue-release-workflow.yml
17

Yes, that's correct.

32

Thanks for catching this. I changed this to use environment variables, which is recommended by the GitHub security documentation. I also test that ' and " are properly escaped.

kwk accepted this revision.Feb 3 2022, 4:02 AM

Minor things in the documentation but I'd say: go for it.

.github/workflows/issue-release-workflow.yml
7–8

COMMIT is <commit>, right? And it probably more than one commit that can be cherry-picked with this command, right? I mean I read the code, so yes.

As a new reader I would ask what the release branch is.

10–12

What is BRANCH in this case? I assume you mean <owner>/<repo>/<branch>?

This revision was landed with ongoing or failed builds.Feb 3 2022, 3:04 PM
This revision was automatically updated to reflect the committed changes.
tstellar marked 2 inline comments as done.
ldionne added a subscriber: ldionne.Feb 3 2022, 3:09 PM

I just read the commit summary but this seems very exciting. I'm glad that folks are starting to use Actions, that will open the door to a lot of goodness elsewhere in LLVM too.