[PATCH] D117386: github: Add actions to automate part of the release workflow

[Konrad Wilhelm Kleine via Phabricator via llvm-commits]

kwk requested changes to this revision.
kwk added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: .github/workflows/issue-release-workflow.yml:13
+    runs-on: ubuntu-latest
+    if: (github.repository == 'llvm/llvm-project') && startswith(github.event.comment.body, '/cherry-pick')
+
----------------
If this gets too long you might as well do this:

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

Your solution is not wrong.


================
Comment at: .github/workflows/issue-release-workflow.yml:24
+        run: |
+          pip install PyGithub GitPython
+          branch=`./llvm/utils/git/github-automation.py --token ${{ github.token }} release-workflow --issue-number ${{ github.event.issue.number }} print-release-branch`
----------------
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:

```lang=diff
- pip install PyGithub GitPython
+ pip install -r requirements.txt
```



================
Comment at: .github/workflows/issue-release-workflow.yml:42
+    runs-on: ubuntu-latest
+    if: (github.repository == 'llvm/llvm-project') && startswith(github.event.comment.body, '/branch')
+
----------------
Same applies here for using multiline `if`s if you want.


================
Comment at: .github/workflows/issue-release-workflow.yml:47
+        run:  |
+          curl -O -L https://raw.githubusercontent.com/$GITHUB_REPOSITORY/$GITHUB_SHA/llvm/utils/git/github-automation.py
+          chmod a+x github-automation.py
----------------
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.


================
Comment at: .github/workflows/issue-release-workflow.yml:49
+          chmod a+x github-automation.py
+          pip install PyGithub GitPython
+
----------------
Another benefit of `requirements.txt` is that you have to maintain dependencies in one place only.


================
Comment at: llvm/utils/git/github-automation.py:39
 
+def setup_llvmbot_git(git_dir = '.'):
+    repo = Repo(git_dir)
----------------
Please have a documentation for these functions.


================
Comment at: llvm/utils/git/github-automation.py:46
+class ReleaseWorkflow:
+
+    def __init__(self, args):
----------------
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 .


================
Comment at: llvm/utils/git/github-automation.py:48
+    def __init__(self, args):
+        self.args = args
+        self.token = args.token
----------------
I suggest to keep this "private": `self._args = args`


================
Comment at: llvm/utils/git/github-automation.py:58
+
+    def get_issue_number(self):
+        return self.args.issue_number
----------------
I suggest to make this a property and the rest of the getters that do simple stuff as well: 

```lang=python
    @property
    def issue_number(self) -> int:
        return self._args.issue_number
```


================
Comment at: llvm/utils/git/github-automation.py:176
+release_workflow_parser = subparsers.add_parser('release-workflow')
+release_workflow_parser.add_argument('--llvm-project-dir', type=str, default='.')
+release_workflow_parser.add_argument('--issue-number', type=int, required=True)
----------------
Please use the `help='...'` parameter to describe what each parameter is doing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117386/new/

https://reviews.llvm.org/D117386