Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add review-bot to require fellows as reviewers #31

Merged
merged 7 commits into from
Sep 24, 2023
Merged

add review-bot to require fellows as reviewers #31

merged 7 commits into from
Sep 24, 2023

Conversation

Bullrich
Copy link
Contributor

@Bullrich Bullrich commented Sep 18, 2023 •
edited
Loading

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

The user's information is fetched always from the chain after every event.

It looks in the fellows data for a field named GitHub and it extracts the handle from there. This resolves #7 (you can find more information about the request there)

This uses pull_request_target for the event, not pull_request. This is a security measure so that an attacker can't steal secrets by changing the workflow file

I added an example configuration for this repository so that we can tweak each individual requirements for the files. Feel free to suggest edits to fulfill your requirements.

  • CI Files
    • All .github/ files.
    • Requires 2 approvals of rank 6 or above
  • Relay files
    • All files inside the relay/kusama or relay/polkadot folder with the exception of the files that end in .adoc.
    • Requires 2 approvals of rank 4 or above
  • System Parachain Files
    • All the files inside system/parachains folder.
    • Requires 2 approvals of rank 2 or above
  • General Files
    • All the files, with the exclusion of the files inside the other rules.
    • Requires 1 approval of rank 1 or above.

All these rules are of type basic. There are more types of rules available but it is intended for teams with different users, and because higher ranks also fulfill requirements for lower ranks, I thought it would be redundant to use those particular type of rules.

Side notes:

@Bullrich
add review-bot to require fellows as reviewers
5d8990b 
Created a Github Action that uses the [Review-Bot app](https://github.com/paritytech/review-bot) to require fellows to review pull requests before allowing the PR to be merged.

The user's information is fetched always from the chain after every event.

It looks in the fellows data for a field named GitHub and it extracts the handle from there. This resolves polkadot-fellows#7 (you can find more information about the request there)

This uses [`pull_request_target`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#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.
xlc
xlc reviewed Sep 18, 2023
View reviewed changes
.github/review-bot.yml Outdated Show resolved Hide resolved
@Bullrich Bullrich mentioned this pull request Sep 18, 2023
Closed
bkchr
bkchr reviewed Sep 18, 2023
View reviewed changes
Copy link
Contributor

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future it would be nice to have some kind of "calculations" based on the rank. So that for example 4 rank two people get the same voting power as one rank 3 or whatever (this is just for illustration, not really put any thought into this).

.github/review-bot.yml Outdated Show resolved Hide resolved
.github/review-bot.yml Outdated Show resolved Hide resolved
.github/review-bot.yml Outdated Show resolved Hide resolved
@xlc xlc mentioned this pull request Sep 19, 2023
Merged
This was referenced Sep 19, 2023
Closed
Merged
Bullrich added a commit to paritytech/review-bot that referenced this pull request Sep 21, 2023
@Bullrich
Added rule type 'fellows' (#85)
bb670b9 
Created a new type of rule named `fellows`. 

This rule has its own evaluation (currently, it is mostly a copy of the
`basic` evaluation). This is done so we can add special cases for rule,
as _suggested by @bkchr in
polkadot-fellows/runtimes#31 (review).

Removed all the ability to request a `rank` in normal rules. I believe
that this is better as we now separate the concerns and simplify logic.
The `or`, `and` and `and-distinct` rule didn't really apply on fellows
because a higher ranking one belongs also to the lower echelons.

This resolves #84 and allows us to experiment better.

Also, this is technically a breaking change, but it has never been
applied on any system, so it wouldn't be breaking an existing system.
@Bullrich
updated file to new version
8ab0ea3 
Version 2.0.0 requires new types and fields.

This will help us so we can develop custom rules in the future.
joepetrowski
joepetrowski reviewed Sep 22, 2023
View reviewed changes
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I thought for the release process we want to push a commit to ./CHANGELOG? That should perhaps be a case and require approval from Fellows and above?

@Bullrich
Copy link
Contributor Author

I thought for the release process we want to push a commit to ./CHANGELOG? That should perhaps be a case and require approval from Fellows and above?

The last rule General files currently encapsulates the CHANGELOG file (it has .*) and it requires 1 review from rank 2 or above. Would this satisfy this requirement or would you recommend that I make a new rule to specifically capture this file?

@mutantcornholio
Copy link
Contributor

mutantcornholio commented Sep 22, 2023 •
edited
Loading

In the future it would be nice to have some kind of "calculations" based on the rank. So that for example 4 rank two people get the same voting power as one rank 3 or whatever (this is just for illustration, not really put any thought into this).

TBH, I feel like this idea is a wrong turn, that would make the bot more complex than it needs to be. Can we actually put some thought into it and provide usecases?

@joepetrowski
Copy link
Contributor

I thought for the release process we want to push a commit to ./CHANGELOG? That should perhaps be a case and require approval from Fellows and above?

The last rule General files currently encapsulates the CHANGELOG file (it has .*) and it requires 1 review from rank 2 or above. Would this satisfy this requirement or would you recommend that I make a new rule to specifically capture this file?

IMO it should be a stronger hurdle than a single rank 2. Would recommend just putting it under the "Relay and system chains" requirements.

@Bullrich Bullrich mentioned this pull request Sep 22, 2023
Merged
bkchr
bkchr approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a first version I would say it is good enough. We can improve in the future.

@bkchr bkchr merged commit ca29673 into polkadot-fellows:main Sep 24, 2023
3 of 5 checks passed
@Bullrich Bullrich deleted the approval-rights-for-fellows branch September 24, 2023 21:17
@ggwpez ggwpez mentioned this pull request Sep 25, 2023
Merged
Bullrich added a commit to paritytech/polkadot-sdk that referenced this pull request Sep 28, 2023
@Bullrich
Added review-bot to fine tune review requirements (#1673)
4384c61 
Created a Github Action that uses the [Review-Bot
app](https://github.com/paritytech/review-bot) to require more fine
tuned requirements to review pull requests before allowing the PR to be
merged.

This uses
[`pull_request_target`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#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:
- paritytech/pr-custom-review#114 -> Proposal for the rewrite
- [Review Bot
Documentation](https://github.com/paritytech/review-bot/blob/main/README.md)

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 paritytech/pr-custom-review#136
- Ability to request fellows instead of teams
- As requested in polkadot-fellows/runtimes#7, this bot has the ability
to request fellows by rank instead of users.
- We currently have 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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joepetrowski joepetrowski joepetrowski approved these changes

@bkchr bkchr bkchr approved these changes

@xlc xlc Awaiting requested review from xlc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge/Approval rights for Fellows
5 participants
@Bullrich @mutantcornholio @joepetrowski @xlc @bkchr