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

Mon Jan 10 11:04:02 PST 2022

kwk added a comment.

In D116762#3226744 <https://reviews.llvm.org/D116762#3226744>, @tstellar wrote:

> In D116762#3226065 <https://reviews.llvm.org/D116762#3226065>, @kwk wrote:
>
>> 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.
>
> It would be nice if the PyGitHub module had wrappers for this already.  What are the advantages of GraphQL vs the REST API?

The advantages of GraphQL that I see is that you can aggregate more than one endpoint into one response. The following example is totally made up but it showcases what you can do: Suppose, you have an issue comment that links to another issue and you want to get the assignee of the other issue. You could totally get this information with one call to the GraphQL by doing nested queries. This also is the reason for why there's no wrapper for this and why you don't need one: GraphQL was designed so that you as a developer can specify what you get in the result, hence there's no way to wrap this.
The GraphQL explorer <https://docs.github.com/en/graphql/overview/explorer> makes it extremely nice to tailor your queries/mutations to your needs by letting you define what (aggregated) fields the result should include.

Try out this example if you want:

  {
    node(id: "IC_kwDOETcFfs47PM2o") {
      ... on IssueComment {
        body
        issue {
          id
        }
        author {
          login
        }
      }
    }
  }

But comparing REST and GraphQL is really comparing apples and oranges. Let me forward this article <https://www.programmableweb.com/news/just-because-github-has-graphql-api-doesnt-mean-you-should-too/analysis/2016/09/21> and a quote from it:

> **Where GraphQL Shines**
>
> Unlike REST, GraphQL is designed to access multiple resources simultaneously.  This means that you are not only able to be more precise in not only retrieving just the data you desire (something that is built into some of today’s modern RESTful APIs), but you are able to do so across multiple resources/ data models (with data joins automatically built in) in a single HTTP (or other applicable protocol) call.
>
> GraphQL is also designed to be incredibly structured (so much so that the order of properties in the response is critical).  This means clients will know exactly what to expect (and in what types) without having to pull in JSON or XML schemas.  It also means the API is much easier to document as the possibilities are limited to its models, not its representations and dynamically managed relationships.

That being said I think programmatically the GraphQL query can easily be verified for correctness outside the program code where the the REST API let's you use auto-completion inside the editor of choice. IMHO with growing complexity of tasks or queries I'd lean towards GraphQL and it's good practice to start small, aka with less complexity.

Sorry for this long debatable answer.

================
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
----------------
tstellar wrote:
> kwk wrote:
> > 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
> I don't think any older content exists, because we are pulling a specific version of the file : GITHUB_SHA.
That's a fair point. Sorry for not seeing that.

================
Comment at: llvm/utils/git/github-automation.py:21
+        self.issue = self.repo.get_issue(issue_number)
+        self.team_name = 'issue-subscribers-{}'.format(label_name).lower()
+
----------------
Sorry for being picky. This is not needed but a though. I'd make this a property to be immutable to change from the outside:

```lang=python
    # In __init__:
    # ...
    self._team_name = 'issue-subscribers-{}'.format(self.label_name).lower()

    @property
    def team_name(self) -> str:
        return self._team_name
```

================
Comment at: llvm/utils/git/github-automation.py:23
+
+    def run(self):
+        for team in self.org.get_teams():
----------------
To help code assists, it's nice to include the return type f this function.

```lang=python
def run(self) -> bool:
```

================
Comment at: llvm/utils/git/github-automation.py:28
+            comment = '@llvm/{}'.format(team.slug)
+            self.issue.create_comment(comment)
+            return True
----------------
Do we ignore any exceptions that can happen? This question is important for future additions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116762