Fall back to main branch protection rules for PRs targeting branches without protection rules

[sarayourfriend]

Fixes

Fixes #1863 by @sarayourfriend

Description

Fall back to main branch protection for PRs that target branches without their own protection rules.

Testing Instructions

Check out the unit test and confirm that it sufficiently covers the new case.

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• [N/A] I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[krysal]

Why skip PRs that do not target main? Isn't there another solution?

I have to look further into the issue but those PR can become stale too.

[sarayourfriend]

Why skip PRs that do not target main? Isn't there another solution?

To determine the number of approvals needed for a PR to be considered "reviewed" (and skipped) we query the branch protection rules. Because no other branches than main have branch protection rules, we don't have a way of deriving the minimum number of required reviews for a PR that targets any other branch.

The alternative to skipping those PRs is to always fall back to 2 required reviews for branches other than main. If that's the preferred approach for reviewers then I am happy to change the PR to that. I don't personally see the need for PR review reminders on PRs that do not target main. In my experience those PRs generally are blocked by their downstream target PR which will inevitably target main and get review reminders.

[krysal]

The alternative to skipping those PRs is to always fall back to 2 required reviews for branches other than main.

Yeah, I think the default would be better. We haven't changed that requirement since it was implemented. Those PRs can be blocked for merging but the review should still happen at some point.

[dhruvkb]

we query the branch protection rules

Instead of falling back to a hardcoded value of 2, I would recommend falling back to whatever is the required number of reviews for main in the queried branch protection rules. This will give us the flexibility to change that in the future, if needed.

[sarayourfriend]

Sounds good.

[sarayourfriend]

@krysal This is ready for re-review now.

[AetherUnbound]

This looks good and makes sense to me!