Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add GHA for custom PR review #10951

Merged
merged 15 commits into from
Apr 8, 2022
Merged

Add GHA for custom PR review #10951

merged 15 commits into from
Apr 8, 2022

Conversation

sergejparity
Copy link
Contributor

@sergejparity sergejparity commented Mar 1, 2022

⚠️ We'll synchronize the merge of this PR with the deprecation of the approval features of processbot (paritytech/parity-processbot#373). Please do not merge it until then.


This PR enables the configuration in pr-custom-review.yml for the rules discussed in paritytech/pr-custom-review#67. Feel free to comment there for any suggestions or disagreements.

In short: Action will monitor PRs for changes defined in rules and if such detected will request review from defined in rule users/teams and will monitor review status by setting by setting Check reviews GitHub Status Check (needs to be enabled in repository settings) on PR which will pass once all needed approvals received.


This is how it requests the relevant reviewers
image

Example output of the job failed by the unfulfilled request for reviewing the 🔒ed line.

@sergejparity sergejparity added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 1, 2022
@sergejparity sergejparity requested a review from a team as a code owner March 1, 2022 10:13
@sergejparity sergejparity requested review from gavofyork and a team March 1, 2022 10:14
@TriplEight TriplEight self-requested a review March 1, 2022 17:45
.github/pr-custom-review.yml Outdated Show resolved Hide resolved
.github/pr-custom-review.yml Outdated Show resolved Hide resolved
.github/pr-custom-review.yml Outdated Show resolved Hide resolved
@sergejparity
Copy link
Contributor Author

sergejparity commented Mar 2, 2022

Check reviews status is failing right now with 404 error, but it is expected behavior. This GHA protecting its configuration against being changed directly in PR by taking into account version residing in master branch. And currently it does not exist in master. It will become operational once this PR will get merged.
Afterwards changes to its configuration should go through the usual approval process to become active.

.github/pr-custom-review.yml Outdated Show resolved Hide resolved
@joao-paulo-parity
Copy link
Contributor

For the record I'll not be able to deprecate the approval rules on processbot (paritytech/parity-processbot#356) until pr-custom-review is available on Polkadot and Cumulus as well. At the same time I intuit that processbot and pr-custom-review do not block nor obstruct each other.

.github/pr-custom-review.yml Outdated Show resolved Hide resolved
@sergejparity
Copy link
Contributor Author

#10968 created to deploy config file for this GHA as this PR cannot proceed without it.

@paritytech-ci paritytech-ci requested review from a team March 3, 2022 09:03
@paritytech-ci paritytech-ci requested a review from a team March 14, 2022 13:43
@gavofyork
Copy link
Member

On the Polkadot side we will need this as well. There we want to protect the Kusama and Polkadot runtime files. Not just the lib.rs. It should at least include the xcm_config.rs and may some of the files from runtime-common. However, we can define this list when the particular Polkadot pr was opened. These changes should be approved by at least 2 people of group of: @gavofyork @shawntabrizi @rphmeier and me.

I'd alter that to: at least two team leads, one of whom is in the group @gavofyork @shawntabrizi, @rphmeier and @bkchr. I think otherwise it adds too much friction.

Same for the 🔒-lines in Substrate.

@joao-paulo-parity
Copy link
Contributor

In light of

I've noticed there hasn't been a consensus on the rules' definitions compared to what's implemented in this PR (which is closer to what we were predicting during paritytech/pr-custom-review#32). #10951 (comment) particularly can be interpreted as divergence from the initial requirements in https://github.com/paritytech/ci_cd/issues/233#issue-1026394650 which, I assumed, would not change; if these requirements would change, since they are related to built-in checks, then not only the rules but also the implementation of pr-custom-review needs to change as well.


Based on how the discussion went so far and how many parties are involved, I also think it's less productive to have this same back-and-forth on each pull request for each repository. I would prefer if we were to define rules for all repositories upfront first and then only move to the implementation once a consensus has been reached, instead of trying to both discuss and implement at the same time like in this PR.


Given the arguments above I've suggested to @TriplEight that it would be more efficient to discuss the rules in English terms, in a separate PR, using the following format as an example:

Repository A:

- Any PR needs 2 core-devs approvals

- Lock line changes needs both:
  - 1 approval from @paritytech/locks-review 
  - 1 approval from one of @UserA, @UserB

- Files which need 2 approvals from @UserA, @UserB, @UserC:
  - ./utils/frame/*
  - ./utils/lib.rs

Repository B:
- ...

Repository C:
- ...

I plan to soon propose a separate discussion PR in https://github.com/paritytech/pr-custom-review, so I suggest we move this PR to Draft in the meantime.

Co-authored-by: João Paulo Silva de Souza <[email protected]>
@paritytech-ci paritytech-ci requested a review from a team April 8, 2022 07:43
.github/workflows/pr-custom-review.yml Outdated Show resolved Hide resolved
Co-authored-by: João Paulo Silva de Souza <[email protected]>
@joao-paulo-parity joao-paulo-parity added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). and removed A0-please_review Pull request needs code review. A7-needspolkadotpr D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 8, 2022
@TriplEight TriplEight merged commit 87ebfdb into master Apr 8, 2022
@TriplEight TriplEight deleted the sk-add-pr-custom-review-gha branch April 8, 2022 15:39
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Add GHA for custom PR review

* Change FILES rule settings

* Update rules according to feedback

* Update .github/pr-custom-review.yml

Co-authored-by: Denis Pisarev <[email protected]>

* CI: PRCR new 🔒 team is locks-review

* CI: rename a confusing step

* Update .github/workflows/pr-custom-review.yml

Co-authored-by: João Paulo Silva de Souza <[email protected]>

* CI: use a proper new team for 

as per discussion with @drahnr it was decided to create a dedicated team for reviewing runtime files

* Update pr-custom-review.yml

* Update .github/workflows/pr-custom-review.yml

Co-authored-by: João Paulo Silva de Souza <[email protected]>

* Update .github/workflows/pr-custom-review.yml

Co-authored-by: João Paulo Silva de Souza <[email protected]>

Co-authored-by: Denis Pisarev <[email protected]>
Co-authored-by: TriplEight <[email protected]>
Co-authored-by: Denis Pisarev <[email protected]>
Co-authored-by: João Paulo Silva de Souza <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Add GHA for custom PR review

* Change FILES rule settings

* Update rules according to feedback

* Update .github/pr-custom-review.yml

Co-authored-by: Denis Pisarev <[email protected]>

* CI: PRCR new 🔒 team is locks-review

* CI: rename a confusing step

* Update .github/workflows/pr-custom-review.yml

Co-authored-by: João Paulo Silva de Souza <[email protected]>

* CI: use a proper new team for 

as per discussion with @drahnr it was decided to create a dedicated team for reviewing runtime files

* Update pr-custom-review.yml

* Update .github/workflows/pr-custom-review.yml

Co-authored-by: João Paulo Silva de Souza <[email protected]>

* Update .github/workflows/pr-custom-review.yml

Co-authored-by: João Paulo Silva de Souza <[email protected]>

Co-authored-by: Denis Pisarev <[email protected]>
Co-authored-by: TriplEight <[email protected]>
Co-authored-by: Denis Pisarev <[email protected]>
Co-authored-by: João Paulo Silva de Souza <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants