Merge/Approval rights for Fellows #7

bkchr · 2023-04-12T10:59:06Z

Fellowship members should be getting approval/merge rights based on their rank like it is done on chain. So, to merge a pull request it will require X members to pass "the voting" to get the approval to merge. The list of fellowship members is maintained on chain, so it would introduce extra overhead to maintain the list on github as well. Like having teams for each rank and then having some CI check that succeeds when "the voting" has passed. As already said, the list and the associated rank is already on chain and we should use this directly in the CI. There exists the pr-custom-review github action from Parity that already overs fine grained control on the approval process. This should be augmented with a script that is using smoldot to fetch the latest list of fellowship members, their ranks and their identities from the latest block. This will require that every fellowship member also provides their github account as part of their on chain identity. Then the CI job will be able to map from a github name to the fellowship rank to do "the voting".

bkchr · 2023-04-12T11:01:23Z

For merging we should then have some kind of github action bot that monitors comments on prs. When someone writes bot merge and all CI checks (also the approval check outlined above) are green, the bot should merge the pr. The bot should maybe do some check that the person writing bot merge is a fellowship member, just to prevent spam or stuff getting merged randomly. (It will still require an approval, but yeah)

bkchr · 2023-05-08T13:38:49Z

Just realized that the identity is stored on the relay chain and the fellowship collectives on the collectives parachain. So, it requires connecting to two chains. However, smoldot should do this internally already to find out what is the best block etc, but I'm not sure if both connected chains can be "used".

mutantcornholio · 2023-05-24T15:43:47Z

The list of fellowship members is maintained on chain, so it would introduce extra overhead to maintain the list on github as well. Like having teams for each rank and then having some CI check that succeeds when "the voting" has passed.

Feels like we could instead do exactly that, and either reuse PRCR, or even use CODEOWNERS.

Synchronising dans from chain to GitHub teams would also bring an ability to grant other types of GH-permissions (e.g. admin, force-push) to fellows of specific dan.
Isn't that also a problem that needs solving?

mordamax · 2023-05-24T15:49:16Z

map from a github name to the fellowship rank to do "the voting".

@bkchr what would be the logic of "voting"? i mean would you for example require only people since dan #3 to be able to approve? is it described anywhere how dan allows/restricts ability to manipulate with github or it's TBD?

bkchr · 2023-05-25T12:59:04Z

Synchronising dans from chain to GitHub teams would also bring an ability to grant other types of GH-permissions (e.g. admin, force-push) to fellows of specific dan.

Isn't as "permissionless" as I proposed, as you would first need to invite someone, but if that is easier, I personally don't care that much. As long as the synchronization would be automatic.

Or only people above some DAN are getting invited as there is more trust in these people.

is it described anywhere how dan allows/restricts ability to manipulate with github or it's TBD?

No, but I also don't see this as that complicated when we have the prerequisites.

shawntabrizi · 2023-07-19T20:45:00Z

Just realized that the identity is stored on the relay chain and the fellowship collectives on the collectives parachain. So, it requires connecting to two chains. However, smoldot should do this internally already to find out what is the best block etc, but I'm not sure if both connected chains can be "used".

I think a map from github name to on-chain address is better managed in this repo.

Updating your identity on-chain unfortunately is not super easy, and will remove your "verified" status unless you pay an identity registrar to re-verify your account.

Some things can be done to make this better, for example giving the fellowship itself an identity registrar spot, which makes sense to me, but I still feel that maintaining a simple lookup table makes more sense here than on-chain.

bkchr · 2023-07-21T08:08:39Z

Some things can be done to make this better, for example giving the fellowship itself an identity registrar spot

We are currently working on this.

Not sure why we should maintain multiple lists which will then require someone to update the list in here as well etc.

arrudagates · 2023-07-24T21:29:36Z

i mean would you for example require only people since dan #3 to be able to approve?

Another point to be raised with this is that disallowing lower ranks to review proposals would hinder even more their ability to rise to upper ranks. In my opinion, all members should be able to participate in GitHub proposal reviews with approval rights, even if that approval has a lower weight compared to higher ranks.

mutantcornholio · 2023-07-24T21:58:07Z

@arrudagates do you mean something like 10 approves from people of dan 3 should be equal to 1 approve of dan 6?

xlc · 2023-07-24T22:14:57Z

People can always comment/review any PR without any permissions. It is just that those won't be considered for merge. I have been reviewing and commenting Substrate PR for years, and that's how I participate and rank up myself. I did not need to ask anyone for permissions to start.

arrudagates · 2023-07-24T22:21:29Z

@arrudagates do you mean something like 10 approves from people of dan 3 should be equal to 1 approve of dan 6?

Yes, these ratios would have to be fine tuned, of course, but I think this would be a fair compromise.
Perhaps the ratios could even be dynamic, 75% of total Dan 1 members equals 1 actual approval, then to merge it needs a specific amount of actual approvals

arrudagates · 2023-07-24T22:23:50Z

People can always comment/review any PR without any permissions. It is just that those won't be considered for merge. I have been reviewing and commenting Substrate PR for years, and that's how I participate and rank up myself. I did not need to ask anyone for permissions to start.

Yes, that's fair and it works out for those outside the fellowship, but at the same time those who are already members of the fellowship should have some power regardless of rank (even if weighted lower than upper ranks), otherwise it hurts the fellowship diversity.

xlc · 2023-07-24T22:42:23Z

I just want to keep it simple. I hear your request and sounds reasonable but at the same time do you really think it is a must have feature for the initial release? Let's get something out first and then consider improving it. Not the other way around.

arrudagates · 2023-07-24T22:49:01Z

I just want to keep it simple. I hear your request and sounds reasonable but at the same time do you really think it is a must have feature for the initial release? Let's get something out first and then consider improving it. Not the other way around.

I agree, but it's important to have these discussions even if not implemented immediately, which is why I commented on it now.

bkchr · 2023-08-28T07:56:39Z

@mordamax what is the status? When do you think we can make use of this bot?

mordamax · 2023-08-28T10:53:40Z

@mordamax what is the status? When do you think we can make use of this bot?

@Bullrich could you please respond? ☝️

Bullrich · 2023-08-28T13:13:55Z

Hi @bkchr!

Currently review-bot is being built with this in mind. It will have the ability to hot swap the source from where we get the "team" members (currently only GitHub, but it is a replaceable module).

Version 1 is almost finished.

I had been in contact with Harry and also updated @josepot on our needs so we can use CAPI.

Harry built a small function for me where I was able to obtain all the users and metadata from the chain.

We have a blocker which requires a different team to resolve:

While we can obtain the chain data, the data needs to be updated so the fellows add their GitHub handles. I don't know how this is could be done, or who to ask, but until this is resolved, we won't be able to obtain their handles.

Could you please provide me some guidance with this?

bkchr · 2023-08-28T13:52:51Z

Version 1 is almost finished.

How much time do you think you will need finish 1.0?

Could you please provide me some guidance with this?

Until this issue is done, we will not have "native" support for the github handle. However, we already support additional data in the identity. We will require to store there ("github", github_handle). If the look at the declaration of Data, we can see that we can at most store up to 32 bytes. github will fit easily in there as a key, however the github handle could maybe be longer than 32 bytes. Thus, I would propose that we use BlakeTwo256(GITHUB_HANDLE) per user there. Then the bot would need to hash the approving accounts as well and check them against this list. Does this sounds reasonable?

Bullrich · 2023-08-28T17:00:01Z

How much time do you think you will need finish 1.0?

Technically speaking I only need to finish 2 more tasks to achieve the same functionality that is being used in the polkadot-sdk repo.

But, I plan to have all my requirements for version 1 ready by the end of next week (which also includes thorough testing).

Until this issue is done, we will not have "native" support for the github handle.

Thanks for passing me that issue, I'll link it to my tasks.

However, we already support additional data in the identity. We will require to store there ("github", github_handle).

This would work if the users update such additional data.

If the look at the declaration of Data, {..} Does this sounds reasonable?

Should be possible, I will need to request the help from the CAPI team, but I don't see any reason for which it wouldn't be achievable.

bkchr · 2023-08-28T21:55:25Z

This would work if the users update such additional data.

Yes for sure, but this is some I will take care of.

Assign reviewers paritytech/review-bot#53

But this is optional? And not really required for the issue here.

Bullrich · 2023-08-29T08:19:05Z

But this is optional? And not really required for the issue here.

Yes, that would only be required to achieve the same functionality that is being used in the polkadot-sdk repo.

But it is completely optional.

bkchr · 2023-09-07T13:13:55Z

@Bullrich what is the status?

Bullrich · 2023-09-07T13:38:36Z

Hi @bkchr!

Review-Bot version 1 has been completed yesterday. I'm currently testing complex cases in paritytech-stg and fixing edge cases.

It currently replicates most of PRCR behaviour (it doesn't assign reviewers yet, that will be in version 1.1.0).

Once I have this version's bugs fixed, I'll be merging it into polkadot-sdk.

That's when I'm going to ask @josepot for help with paritytech/review-bot#61

Any updates with the identity field or with paritytech/polkadot-sdk#179?

bkchr · 2023-09-07T13:42:42Z

We will require to store there ("github", github_handle)

We will start with this, as I said. So, we don't need paritytech/polkadot-sdk#179 for now.

Once I have this version's bugs fixed, I'll be merging it into polkadot-sdk.

I'm interested in this repo here and nothing else.

(it doesn't assign reviewers yet, that will be in version 1.1.0)

We don't need this, we need paritytech/review-bot#61

bkchr · 2023-09-13T10:15:10Z

@Bullrich it's me again ;) Status?

Bullrich · 2023-09-13T15:15:42Z

Hi @bkchr!

Yesterday @josepot showed us how he is able to fetch the github handle with his library.

I'm currently close to achieve the same functionality using polkadot-js (which will be replaced by the other library in the future).

We have been able to extract the github data in both cases, so we are almost there! I'll comment here once we have the final PR

Bullrich · 2023-09-13T18:27:55Z

@bkchr I have a draft Pull Request that is fetching the fellow's handles here: paritytech/review-bot#79

Bullrich · 2023-09-15T16:54:54Z

I have two more sub tasks:

I will finish those today and have the release ready.

In the meanwhile, do you know what kind of rules will you be requiring?

the config is extremely similar to PRCR, so I can adapt it to review-bot.

bkchr · 2023-09-15T21:24:53Z

For the pr adding the bot, I propose we let everyone from rank 2 voting. Rank - 1 should be your "voting power". All approvals together should be above 20 voting power. We can then discuss the proper configuration in your pr.

bkchr · 2023-09-17T18:49:39Z

@Bullrich I see all your tasks are finished? Could you please open a pr to add the bot to the repo?

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.

Bullrich · 2023-09-18T01:30:39Z

@Bullrich I see all your tasks are finished? Could you please open a pr to add the bot to the repo?

@bkchr find the Pull Request here:

add review-bot to require fellows as reviewers #31

I added example cases, but feel free to request the changes that you find suitable.

* add review-bot to require fellows as reviewers 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 #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. * removed non existent directory * Apply suggestions for required rankings Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * merged system parachains with relay files * updated minFellowsRank for .github files Co-authored-by: Bastian Köcher <git@kchr.de> * updated file to new version Version 2.0.0 requires new types and fields. This will help us so we can develop custom rules in the future. * added CHANGELOG to 'Relay and System files' rule --------- Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

@the-right-joyce

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

bkchr added the enhancement New feature or request label Apr 12, 2023

Bullrich mentioned this issue Aug 28, 2023

Obtain handles from the fellows paritytech/review-bot#61

Closed

Bullrich mentioned this issue Sep 18, 2023

add review-bot to require fellows as reviewers #31

Merged

Bullrich mentioned this issue Sep 22, 2023

Added review-bot to fine tune review requirements paritytech/polkadot-sdk#1673

Merged

bkchr closed this as completed in #31 Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge/Approval rights for Fellows #7

Merge/Approval rights for Fellows #7

bkchr commented Apr 12, 2023

bkchr commented Apr 12, 2023 •

edited

Loading

bkchr commented May 8, 2023

mutantcornholio commented May 24, 2023

mordamax commented May 24, 2023

bkchr commented May 25, 2023

shawntabrizi commented Jul 19, 2023

bkchr commented Jul 21, 2023

arrudagates commented Jul 24, 2023

mutantcornholio commented Jul 24, 2023

xlc commented Jul 24, 2023

arrudagates commented Jul 24, 2023

arrudagates commented Jul 24, 2023

xlc commented Jul 24, 2023

arrudagates commented Jul 24, 2023

bkchr commented Aug 28, 2023

mordamax commented Aug 28, 2023

Bullrich commented Aug 28, 2023

bkchr commented Aug 28, 2023

Bullrich commented Aug 28, 2023

bkchr commented Aug 28, 2023

Bullrich commented Aug 29, 2023

bkchr commented Sep 7, 2023

Bullrich commented Sep 7, 2023

bkchr commented Sep 7, 2023

bkchr commented Sep 13, 2023

Bullrich commented Sep 13, 2023

Bullrich commented Sep 13, 2023

Bullrich commented Sep 15, 2023

bkchr commented Sep 15, 2023

bkchr commented Sep 17, 2023

Bullrich commented Sep 18, 2023 •

edited

Loading

Merge/Approval rights for Fellows #7

Merge/Approval rights for Fellows #7

Comments

bkchr commented Apr 12, 2023

bkchr commented Apr 12, 2023 • edited Loading

bkchr commented May 8, 2023

mutantcornholio commented May 24, 2023

mordamax commented May 24, 2023

bkchr commented May 25, 2023

shawntabrizi commented Jul 19, 2023

bkchr commented Jul 21, 2023

arrudagates commented Jul 24, 2023

mutantcornholio commented Jul 24, 2023

xlc commented Jul 24, 2023

arrudagates commented Jul 24, 2023

arrudagates commented Jul 24, 2023

xlc commented Jul 24, 2023

arrudagates commented Jul 24, 2023

bkchr commented Aug 28, 2023

mordamax commented Aug 28, 2023

Bullrich commented Aug 28, 2023

We have a blocker which requires a different team to resolve:

bkchr commented Aug 28, 2023

Bullrich commented Aug 28, 2023

bkchr commented Aug 28, 2023

Bullrich commented Aug 29, 2023

bkchr commented Sep 7, 2023

Bullrich commented Sep 7, 2023

bkchr commented Sep 7, 2023

bkchr commented Sep 13, 2023

Bullrich commented Sep 13, 2023

Bullrich commented Sep 13, 2023

Bullrich commented Sep 15, 2023

bkchr commented Sep 15, 2023

bkchr commented Sep 17, 2023

Bullrich commented Sep 18, 2023 • edited Loading

bkchr commented Apr 12, 2023 •

edited

Loading

Bullrich commented Sep 18, 2023 •

edited

Loading