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

[Tom Stellard via Phabricator via llvm-commits]

tstellar added inline comments.


================
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`
----------------
kwk wrote:
> 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.
OK, thanks for the explanation.  I've added the requirements.txt file.


================
Comment at: llvm/utils/git/github-automation.py:146
+
+    def create_branch(self, commits:list):
+        """
----------------
kwk wrote:
> 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}
I tried this, but python gave an error saying this wasn't supported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117386