Adding pr-opened-by option to required-review action

[paveldedik]

I forked https://github.com/Automattic/action-required-review to https://github.com/paveldedik/action-required-review

It adds the following option (we have a need for that in our company):

requirements: |
  - paths: unmatched
    teams:
      - maintenance
      - pr-opened-by:
        - devops

This means the required review is unnecessary if the DevOps team opened the PR. It might seem weird to allow that, but consider the following scenario:

• we have files (e.g. migrations) that a team lead should approve
• if the team lead opened the PR, we don't need another team lead's approval (they probably don't have the context anyway), we want just someone's review in that case

Would you be interested in adding this to the main action? This option is kind of weird and the solution you can check here is not the cleanest probably: paveldedik/action-required-review@f35d16b#diff-653820f7683f5805a61ece3efebfe7eb3617f3e8c1c7319f67b7a4096f7c389e

So maybe it should stay in the fork. But if you want, I can send the PR.

[jeherve]

Thanks for starting the discussion!

if the team lead opened the PR, we don't need another team lead's approval (they probably don't have the context anyway), we want just someone's review in that case

I assume that in this case, just setting the requirement to getting a review from someone else in devops wouldn't work? It would satisfy the need for a review.

[paveldedik]

Thank you for the answer.

I should probably provide a better example:

teams:
  - dev-teamleads
  - pr-opened-by:
      - dev-teamleads

In this case, I want to have a review from at least one team lead or I want to have a PR opened by a team lead (+ another review from someone, but that doesn't have to be specified here as GitHub can be already configured to require at least one review).

I cannot make this condition without the pr-opened-by key. If a team lead opens the PR, they cannot review their own pull request (and we don't want that) so they have to write another team lead (who might not be the best choice as they don't have the context).

[paveldedik]

I think the problem is even more obvious, when there is only one member in a group (or the group is just a user).

I can imagine a set up where all adr/** files have to be reviewed by an architect. So when the architect opens a PR, they have no way of making the pipeline pass as they cannot review their own pull request.

[anomiex]

I can think of two ways this might be done.

• A top-level flag on the requirement that tells it to effectively count the committer as having "reviewed" for the purpose of satisfying the requirement. Advantage: Easier to configure. Disadvantage: Less flexible, you can't say only some committers count but not others.
• Much like in the linked fork, although I'd probably just have the "pr-opened-by" replace the reviewers array for the calls to sub-filters instead of passing the committer as a separate parameter and then behaving differently at the string level.

[anomiex]

Or a third option, might be to make it "pr-reviewed-or-opened-by" instead of just "pr-opened-by" that adds the committer as a "review" for sub-filters. That would save having to repeat the team twice, e.g.

teams:
  - devs
  - pr-reviewed-or-opened-by:
      - dev-teamleads

would be the same as the proposed

teams:
  - devs
  - dev-teamleads
  - pr-opened-by:
      - dev-teamleads