search

paritytech / pr-custom-review

GitHub Action for complex pull request approval cases that are not currently supported by the Branch protection feature in GitHub.
MIT License
8 stars 4 forks source link

got some feedback from an initial review #1

Closed TriplEight closed 2 years ago

TriplEight commented 2 years ago

README:

The frontline might have been PR Custom Review + GH action

# Multiple groups with can be specified - not finished line

is it possible to mention a github group using org/group notation or does it infer some org? Either way, it's not clear from an example.

Please mention in the comment which permissions are required from the token, I assume GITHUB_TOKEN is needed for reading users/groups from an org.

Please add the instructions on what shall we do with the github repo configs (leave a review requirement)

A snippet from an example:

- name: reviewers_group2
      minimum: 2
      from:
        person:
          - user3
          - user4

do user3 and user4 have to be from reviewers_group2? can I list just users, no groups (I mean github groups everywhere)? can I list just groups, with minimum reviewers, and no users? can I namely exclude users from a group in a config, i.e. they're ooo? - optional, can exclude a person from a github group, this doesn't have to be saved in code

Another snippet from an example:

on:
  pull_request:
    types:

I think makes sense to explain shortly why each of types is needed

paritytech/pr-custom-review@master let's mint a release tag!

On Diagram: what's PRR? Pull Request Review?

LICENSE - I think it's not done by GitHub. I.e. https://github.com/paritytech/srtool/blob/613d650f61d52a949c8ce8c0e2a938be41bcb562/LICENSE or https://github.com/paritytech/companion-for-processbot-staging/blob/master/LICENSE :)

https://github.com/paritytech/pr-custom-review/blob/master/src/review_gatekeeper.ts either rename a file or mention the source

joao-paulo-parity commented 2 years ago

@TriplEight one thing I don't get from this feedback is what a "group" refers to. GitHub does not have a "group" entity. Is it talking about teams?

By the types: example which does not exist in the README, I infer this review is outdated. I tried to move the actionable items to other issues:

can I list just users, no groups (I mean github groups everywhere)? can I list just groups, with minimum reviewers, and no users?

Added some scenarios as test cases to https://github.com/paritytech/pr-custom-review/issues/18

do user3 and user4 have to be from reviewers_group2?

I think the term "group" is misused and causing the confusion here. "reviewers_group2" in that example is the rule name. https://github.com/paritytech/pr-custom-review/issues/19

On Diagram: what's PRR? Pull Request Review?

Moved to https://github.com/paritytech/pr-custom-review/issues/20

Please add the instructions on what shall we do with the github repo configs (leave a review requirement)

This one I did not understand, but it would be the last actionable item I'm able to extract from this at the moment.

TriplEight commented 2 years ago

Group == team, sorry.

On types: I meant GitHub ones

on:
  pull_request:
    types:

it exists in README.

This one I did not understand, but it would be the last actionable item I'm able to extract from this at the moment.

That's to explain how to setup the branch protection rules in the repository.

Will comment in PRs you've mentioned.

joao-paulo-parity commented 2 years ago

For the event types I've created https://github.com/paritytech/pr-custom-review/issues/21

That's to explain how to setup the branch protection rules in the repository.

There's a collapsed screenshot showing how to do that ("Expand screenshot"). In the new README of #12 I've put the image directly in the document, you can see it at https://github.com/joao-paulo-parity/pr-custom-review/tree/readme#github-repository-configuration--.

I think we've extracted all the action points here, thus I'm closing this issue. If there's any unattended feedback I suggest we move them to new issues.