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

Wed Jan 26 08:04:29 PST 2022

kwk added a comment.

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.

================
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`
----------------
tstellar wrote:
> kwk wrote:
> > 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
> > ```
> > 
> 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.
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 at work:~$ python3 -m venv $PWD/myenv
kkleine at work:~$ cd myenv/
kkleine at work:~/myenv$ source bin/activate
kkleine at 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.

================
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
----------------
tstellar wrote:
> tstellar wrote:
> > kwk wrote:
> > > 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.
> > 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.
> I dropped this, the shallow checkout seems to be fast enough.
Thank you. 

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

================
Comment at: llvm/utils/git/github-automation.py:146
+
+    def create_branch(self, commits:list):
+        """
----------------
I suggest to change the interface from

```lang=python
def create_branch(self, commits:list):
```

to 

```lang=python
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:

{F21861611}

================
Comment at: llvm/utils/git/github-automation.py:148-149
+        """
+        This function attempts to backport `commits` into the branch associated
+        with `self.issue_number`.
+
----------------
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.

================
Comment at: llvm/utils/git/github-automation.py:210-211
+            if command == 'cherry-pick':
+                self.create_branch(args.split())
+                return True
+
----------------
Why not  `return self.create_branch(args.split())`?

================
Comment at: llvm/utils/git/github-automation.py:218-219
+                    branch = m.group(3)
+                    self.create_pull_request(owner, branch)
+                    return True
+
----------------
Same here, why not `return self.create_pull_request(owner, branch)`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117386