[PATCH] D116762: workflows: Make issue-subscriber more robust for labels with special characters

[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.

Since this looks like the beginning of something new and big I highly suggest to consider using GraphQL instead of doing the many REST calls that are needed. Especially when you drive stuff from a github action you have almost everything you need right at your finger tips. I mean all the global IDs for issues and alike. I'm working on something for another project and here's my example of how one could encapsulate the github functionality into classes with easy usage: https://gist.github.com/kwk/c89b6a3e5eb40487fed78f226e982fcc#file-graphql-py-L138 .

Anyway this is just a thought.



================
Comment at: .github/workflows/issue-subscriber.yml:15
+      run:  |
+        curl -O -L https://raw.githubusercontent.com/$GITHUB_REPOSITORY/$GITHUB_SHA/llvm/utils/git/github-automation.py
+        chmod a+x github-automation.py
----------------
I suspect that checking out the repo is not an option here? 

But anyways, if you absolutely need curl to download the script, then be aware of some issues with this approach. I noticed that curl sometimes queries an older version of the content than currently is on github. The solution could be this:

```
curl \
   --compressed \
   -s \
   -H 'Cache-Control: no-cache' \
   https://raw.githubusercontent.com/$GITHUB_REPOSITORY/$GITHUB_SHA/llvm/utils/git/github-automation.py?$(uuidgen)
```

You might wonder about the `--compressed` and caching options or even about
the UUID being attached to the URL at the very end. These are all ways to
 ensure we get the freshest of all versions of the file on github. I got the inspiration for this from here:
https://stackoverflow.com/questions/31653271/how-to-call-curl-without-using-server-side-cache?noredirect=1&lq=1


================
Comment at: llvm/utils/git/github-automation.py:19
+
+    def __init__(self, args):
+        self.repo = github.Github(args.token).get_repo(args.repo)
----------------
Being picky here, although this looks straight forward, I highly suggest that we use typed python:

```lang=python
def __init__(self, token:str, repo:str, issue_number:int, label_name:str):
```

This makes functions so much more readable IMHO.


================
Comment at: llvm/utils/git/github-automation.py:35-39
+def get_default_repo():
+    default = os.getenv('GITHUB_REPOSITORY')
+    if not default:
+        default = 'llvm/llvm-project'
+    return default
----------------
`os.getenv()` is overloaded and accepts a second value to be the default if the environment variable is `None`.

```lang=python
def get_default_repo():
    return os.getenv('GITHUB_REPOSITORY', 'llvm/llvm-project')
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116762