Skip to content

Relax PR merge rules, enforce them via CI - #91

Open
kolyshkin wants to merge 2 commits into
opencontainers:mainfrom
kolyshkin:review-policy
Open

Relax PR merge rules, enforce them via CI#91
kolyshkin wants to merge 2 commits into
opencontainers:mainfrom
kolyshkin:review-policy

Conversation

@kolyshkin

@kolyshkin kolyshkin commented Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is the same as opencontainers/runc#5512 (see there for the rationale
and discussion), with the only differences being local specifics:

  • there is no CHANGELOG.md, so README.md is the only top-level *.md
    file which is not a governance file;
  • the existing MAINTAINERS_GUIDE.md wording is merged with the new text.

The check itself is not copied, but used from runc as a reusable workflow
(see opencontainers/runc#5573), pinned to a commit. Dependabot is told not
to bump it, as a bump may change the rules.

After merging

A repo admin needs to update the main branch protection:

  1. Add review-policy / review-policy (from GitHub Actions) as a required
    status check.
  2. Lower the required number of approvals from 2 to 1.

Once the check is required, open PRs won't have it until someone reviews
them (a comment-only review is enough).

Note this is a governance change, so it needs two LGTMs.

@kolyshkin
MAINTAINERS_GUIDE: clarify and relax PR merge rules
5ef1342 
Many pull requests stay open for weeks waiting for a second review,
which slows down development considerably.

Spell out the merge rules, and relax them for maintainers: a pull
request authored by a maintainer can be merged with a single LGTM
from another maintainer, provided it has been open for review for at
least 7 days (14 days for large changes, i.e. more than 100 lines),
so that other maintainers had a chance to object.

Also, allow routine pull requests (opened by Dependabot, or only
changing documentation and/or CI configuration) to be merged with a
single LGTM.

Neither of the above applies to governance changes, i.e. changes to
the project rules (such as this document) or their enforcement, which
always require two LGTMs.

While at it, clarify that:
 - an LGTM is a pull request approval;
 - a maintainer requesting changes blocks the merge.

This is the same as done in runc (see opencontainers/runc#5512).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
kolyshkin requested a review from a team as a code owner October 19, 2026 08:04
AkihiroSuda
AkihiroSuda reviewed Oct 22, 2026
View reviewed changes
Comment thread .github/workflows/review-policy.yml

@AkihiroSuda AkihiroSuda Oct 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we load an action yaml from the runc repo?

@kolyshkin kolyshkin Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea actually; let me see if that's possible

@kolyshkin kolyshkin Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda looks like it's possible to have a single action for two repos but it needs a simple change in runc repo; opened #5573, PTAL

Once merged, here we pin it by sha and use from here like this:

  on:
    pull_request_review:
      types: [submitted, edited, dismissed]
  permissions: {}
  jobs:
    review-policy:
      permissions:
        contents: read
        pull-requests: read
      uses: opencontainers/runc/.github/workflows/review-policy.yml@<sha>

@kolyshkin kolyshkin Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented

@kolyshkin kolyshkin mentioned this pull request Oct 23, 2026
Merged
opensource-fork-sync Bot pushed a commit to cla7aye15I4nd/opensource-opencontainers__runc that referenced this pull request Oct 23, 2026
@kolyshkin
ci/review-policy: allow to use from other repos
a254e0e 
Make the review-policy workflow reusable (via workflow_call), so other
opencontainers repositories with the same merge rules (such as
opencontainers/cgroups, see opencontainers/cgroups#91) can call it
instead of carrying a copy.

The calling repository gets the same logic, which is fine, since:
 - CHANGELOG.md and vendor/ exceptions are no-ops where there's no
   such file/directory;
 - the merge_group trigger is the caller's, and it's harmless when unused.

The only difference was CODEOWNERS, which cgroups treats as a governance
file. Add it here, too, as it does define who reviews what.

Document how to use it from another repository.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
ci: add review-policy check
c6b0f75 
Add a check enforcing the pull request merge rules from
MAINTAINERS_GUIDE.md, to be used as a required status check (in
addition to the GitHub's required number of approvals, which can not
depend on who the author is, and also counts approvals from anyone
having write access, not just from current maintainers).

The check is a pull_request_review workflow with a read-only token,
which evaluates the current state of the pull request using the GitHub
API. No other triggers are used, since branch protection requires the
latest run for each event type to pass, so a failed run triggered by,
say, a push would keep blocking the merge after a review fixes it.

As a result, the check needs a review (a comment-only one is fine) to
be run after a push, and it needs to be re-run manually once a review
period is over.

Note that since the workflow runs from the pull request, a pull request
can change it. This is the price for not using privileged triggers
(pull_request_target or workflow_run) and write permissions. To
mitigate, the workflow is a governance file (so changing it requires
two LGTMs), and GitHub's own required approval stays as a backstop.

The check itself is the runc one (see opencontainers/runc#5512), used
as a reusable workflow (made so in opencontainers/runc#5573), pinned to
a commit. Dependabot is told not to bump it, since a bump may change the
rules, so it should be a deliberate (and governance) change.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
kolyshkin requested a review from AkihiroSuda October 23, 2026 18:53
kolyshkin
kolyshkin commented Oct 23, 2026
View reviewed changes

@kolyshkin kolyshkin left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trigger reivew-policy)

@kolyshkin

kolyshkin commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

(trigger reivew-policy)

it works! https://github.com/opencontainers/cgroups/actions/runs/40007115064/job/120597720296?pr=91

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah Awaiting requested review from thaJeztah
@haircommander haircommander Awaiting requested review from haircommander
@dims dims Awaiting requested review from dims
@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kolyshkin @AkihiroSuda