Added review-bot to fine tune review requirements

[Bullrich]

Created a Github Action that uses the Review-Bot app to require more fine tuned requirements to review pull requests before allowing the PR to be merged.

This uses pull_request_target for the event, not pull_request. This is a security measure so that an attacker doesn’t have access to the secrets.

All the rules have been copied from the original .github/pr-custom-review.yml file.

I want to clarify, this particular commit is not intended to replace PRCR yet.

Advantages it brings over PRCR

Most of the features available in PRCR have been duplicated and enhanced. For a complete detailed write up, please see:

• Rewrite PR Custom Review and Prepare for Fellowship pr-custom-review#114 -> Proposal for the rewrite
• Review Bot Documentation

The most important features are:

• include and exclude fields now accept an array, making it easier to read the regular expressions.
• Ability to skip a rule
  • We can set that PRs coming from a particular user or team will cause the rule to be skipped.
  • This is used in the Audit rule, which was requested by @the-right-joyce.
  • This resolves Add a rule for SRLabs audits pr-custom-review#136
• Ability to request fellows instead of teams
  • As requested in Merge/Approval rights for Fellows polkadot-fellows/runtimes#7, this bot has the ability to request fellows by rank instead of users.
  • We currently have add review-bot to require fellows as reviewers polkadot-fellows/runtimes#31 which is using that feature.

Aside from all the rules available in PRCR I have added a particular rule to lock the review-bot files and require a review from the locks-review team, the @paritytech/ci team and the @paritytech/opstooling team to ensure that the file has been written correctly.

Next steps

The next steps will consist on paritytech/review-bot#53, once this issue has been resolved, and review-bot has worked without any issues on this repository for a while, we will upgrade it to be able to fully replace PRCR.

[Bullrich]

Question for the reviewers:
I set the label to R0-silent and didn't write a .prdoc.

Was this the correct course of action? I supposed that a PR that modifies the CI system is not part of the exposed changelog.

[kianenigma]

re. both this and paritytech/pr-custom-review#136, I don't get why there is no way to circumvent a change that is purely aesthetic and should not be audited?

[kianenigma]

Okay, I found that all core-devs is capable of skipping the rule.

two comments:

1. allowing all core devs to skip the check is basically equal to removing the check. All developers in engineering are in the core devs team. If everyone has the key to something, then no one has the key to that. If everyone can skip this, what is the point of having it?
2. tbh I kinda liked the label based approach a bit better in the sense that when I was reviewing a PR I could see at a glance if the author is gonna wait for an audit or not. How can I do that now?

[Bullrich]

1. allowing all core devs to skip the check is basically equal to removing the check. All developers in engineering are in the core devs team. If everyone has the key to something, then no one has the key to that. If everyone can skip this, what is the point of having it?

I believe that this was intentional. If I am not misinterpreting @the-right-joyce, she wants to audit only PRs made by external people, and ignore ones from internals.

2. We can still provide such request. I'm currently working on a side project for the security team, and adding a label would be a minor task.

[kianenigma]

I believe that this was intentional. If I am not misinterpreting @the-right-joyce, she wants to audit only PRs made by external people, and ignore ones from internals.

This is too lenient, IMO. All PRs done internal or external, if they will insignificantly alter the Polkadot runtime down the road, must be audited.

[mordamax]

@kianenigma isn't Polkadot runtime going to be removed quite soon?

[kianenigma]

It is already technically removed, but it doesn't change anything about the rule. Some pallets and codes and configs are used in the Polkadot runtime in the fellowship, some or not.

[0xJayPi]

It looks like this bot introduced SRLabs as a required reviewer, which is blocking merging of some PRs atm. I understand this was an unexpected behaviour. Could you confirm?

[mordamax]

@0xJayPi it was intended behavior at some level, please refer to paritytech/pr-custom-review#136