Rewrite PR Custom Review and Prepare for Fellowship

[Bullrich]

Rewrite of PR-Custom-Review

Let’s rewrite PR-Custom-Review 📝.

Why?

The code itself is very hard to maintain.

Flow of the current state of the app

This are my notes of the method [run checks](https://github.com/paritytech/pr-custom-review/blob/master/src/core.ts#L109) located in the core file. I tried to visualize the flow of the application to understand it better. 

I can go in detail if you anyone wants me to (there is a flow diagram showing how it works internally) but, to summarize it:

• The whole logic of the app is in a single method (which is almost 800 lines long)
• There is no abstraction between logic and server calls
  • It randomly calls the GitHub api when it needs to.
• There are sub routines inside sub routines invoked by other subroutines
  • This makes it very difficult to read the code or extend it.
• The code rules try to find the perfect match by brute forcing a correct match
  • While this is logical, code wise it is very difficult to read.
• It encourages using an external server to fetch the required information.
  • This is not needed anymore as a new event has been added: pull_request_target
  • pull_request_target runs a GitHub action in a Pull Request using the action file from the root to stop it from being affected if a malicious user modified the action file.
  • Ideal when you only want to read code and not execute/build anything in the target repository
    • More information in: Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests.
• There are no unit tests, just integration tests.
  • This is because the code is combined, so the API must be mocked all the time and there are no unit components.
  • The logic for validating is too wrapped with the API calls.
  • I don’t even know how to start reading the tests.
• The project is over-engineered.
  • It does absolutely everything you will ever need, but the complexity went to the clouds because of that.
  • For example, paritytech/pr-custom-review#104 is an easy task, but that requires to:
    • Update the brute force algorithm
    • Update the nock for the calls
    • Update the array with the test cases.
  • There are unreadable configuration files with an over-combination of cases.
    • Instead of using regex to handle 70 different types of files, why not use an array to put each condition? It makes it more readable!
  • ~The [action review file rule] has a big flaw:~
    • ~It reads itself to find out who can modify itself.~
    • ~If a user updates that file, and who has to review it, the review itself can be circumvented to using different user/teams.~
    • This should use CODEOWNERS instead as it wouldn’t collide, have this security issue.
    • Edit: This is actually taken into consideration

      The how:

      First of all, let’s write down all the necessary uses that this app has. For this, I will use the configuration files of:

• Polkadot
• Substrate

By necessary, I refer to features that are actually being used. If there is a feature it is not currently in use, I prefer to remove it and only add it if requested. I will also remove the action review file rule, as the security of this file shouldn’t be managed by the file itself.

App features

The features that are currently in use are the following:

Rules

The action allows passing a set of rules. These rules have the following fields:

• Set conditions for a rule to be executed. This can be either to only include given files, or to exclude given files (if all files are included).
  • This files must be set as a regular expression.
  • I propose to replace it with an array of glob patterns (more in a subsection)
• Teams
  • Which teams are required to review for this rule to pass.
  • The team members will be requested to review either individually or as a team.
• Minimum approvals
  • How many approvals matching a rule must be executed.
• BASIC, AND & AND DISTINCT rules.
  • PR Custom Review has 4 different types of rules, but only the 3 mentioned are being used.
  • The OR rule is not in use.
  • We should improve the rule complexity and make it easy to understand what it does.
• Prevent review
  • Removes a team/user from being requested to review PRs.

    Features not used

    These are all features that are available but are not being used by any of the projects. For complexity’s sake, I recommend removing all that is not being used currently and only adding it if it is necessary/requested.

• Lock reviews 🔒
  • Requests that a team reviews a PR if a particular file/files are modified.
  • Currently, it is not being used in Polkadot nor Substrate
  • This is literally the same functionality that CODEOWNERS provide.
• action-review-team
  • This feature is being used, but it can be removed and replaced by CODEOWNERS (as it does the same thing).
  • I welcome a debate on this subject.
• diff comparison.
  • The app allows verifying if there is a change in the diff, or if a particular file inside a pattern changed. The diff part is not being used (probably because the file change is easier to understand and encompass more).

    The changes

    These are the things I propose that should change and/or be improved based on the current usage, on developer experience, or that have been requested.

    Verbosity

    The current implementation logs aren’t straightforward to read. We will use GitHub Action’s Summaries to provide a clear text of what is missing.

    Code structure

    The code will be divided into different areas:

• The GitHub api call
  • This will be a class where all the calls are executed
  • By having it as a class, we can mock it for tests.
• The validation class
  • This will handle all the validation checks based on our requests.
• The action class
  • This class will use both the GitHub api class and the logic class to get all the logic.

The idea is to be able to properly test only the validator or the GitHub api without needing to constantly run integration tests. With a properly defined structure and abstraction, extending the code and maintaining it will be easier.

Replace RegEx for Glob Pattern

I recommend that, for the include/exclude section, instead of having regular expressions we use instead an array glob pattern to be used. Example:

# before
include: ^runtime\/(kusama|polkadot)\/src\/.+\.rs$
# after
include:
  - runtime/polkadot/src/**/*.rs
  - runtime/kusama/src/**/*.rs

It makes the code more readable, and a glob pattern is easier to recognize than a regex.

Produce an output of users to be requested

If we decide to implement paritytech/review-bot#92 as a standalone action, we can produce an output for the job and use that job in a different step.

Obtain teams from paritytech/list-team-members

We have an action that can fetch the required team members. This could simplify the logic of the action by combining it with that other action, the same way that Stale Issues Finder uses that output.

This will allow us to easily switch the “teams” for users obtained from the chain data As requested by Gavin:

And with the planned move of the runtimes to the fellowship, we should also alter the locks reviews so that they take into account the fellowship rank via a smoldot query.

Remove the server

We can use pull_request_target to automatically run the action with secrets without the risk of having users trying to get out secrets.

• This action only runs using the action file from the root repository, and is recommended to use only if you are parsing the content of a pull request, not executing the code.
• Don’t do npm install && npm run build in a PR with an action using this event.
• We would only be reading the content of the PR (not executing it) and assigning reviewers, so this event is perfect for our case.

  Convert action into Dockerfile

  This will greatly improve the execution time. This has been done for our other GitHub actions.

[rzadp]

Just one thing I spotted without commenting on anything else:

Lock reviews 🔒 Requests that a team reviews a PR if a particular file/files are modified. This is literally the same functionality that CODEOWNERS provide.

I believe that's incorrect, the 🔒 locks in PRCR refer to lines, not files. So this functionality is not provided by CODEOWNERS.

I remember searching for usages and remember that it was not used, however this functionality is specifically mentioned in https://github.com/paritytech/pr-custom-review/issues/104 so we should double check that.

[Bullrich]

I believe that's incorrect, the 🔒 locks in PRCR refer to lines, not files. So this functionality is not provided by CODEOWNERS.

Regarding this, you are right, but also the whole file after the line:

Locks touched: Lines which have a lock emoji (🔒) or any line directly below a lock emoji require

• 1 approval from locks-review-team
• 1 approval from team-leads-team

Though, this is only being used at the beginning of a the configuration files:

• Polkadot
• Substrate

I remember searching for usages and remember that it was not used, however this functionality is specifically mentioned in paritytech/pr-custom-review#104 so we should double check that.

Good catch with that. I didn't notice that part.

I'm not very fond of that particular functionality because reading the whole files for the locks adds a lot of complexity, and this could be replaced with an AND rule:

  - name: Locked files
    condition:
    # you add the file you want here instead of adding the lock functionality
    include:
      - runtime/polkadot/src/**/*.rs
      - runtime/kusama/src/**/*.rs
    all_distinct:
      - min_approvals: 1
        teams:
          - locks-review-team
      - min_approvals: 1
        teams:
          - team-leads-team

Another reason for which I would comment against the lock functionality is that it could be that someone adds a comment and that would break lock the file, as the whole lock functionality is not really known.

// This functions locks the user key from third party providers 🔑➡🔒

println!("Transaction locked 🔒");

Also, you can think that a 🔓 is a 🔒 and confuse people.

[Bullrich]

Meeting notes

• [ ] Discuss the assignment logic
  • How do we want to assign users?
  • Direct assign every single possible user?
  • Round robin?
  • Create teams and assign teams & sync them on every action execution?
• [x] Implement the fetch of members from teams in the code
• [x] Use an array of regex(es?)
• [x] Use GitHub's implemention in opstooling-js instead of a GitHub api call class
• [ ] Implement the lock
  • Do we need a lock in the screen?
  • Can we use a different think that one emoji?
• [x] Understand why there is a need for "prevent review" instead of just removing review assignment of code owners

[mordamax]

Closed this phase as done