Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 187 additions & 0 deletions .github/workflows/review-policy.yml

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?

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

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>

Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
# Enforce the pull request merge rules described in MAINTAINERS_GUIDE.md
# ("Who decides what?"). This job is a required status check.
#
# It only runs when a review is submitted, edited, or dismissed, and evaluates
# the current state of the pull request. Other events (pull_request) are not
# used on purpose: branch protection requires the latest run for each event
# type to pass, so a failed pull_request run (say, "needs an LGTM") would keep
# blocking the merge after a review fixes it. This means:
#
# - after new commits are pushed, the check is missing (so the pull request
# can't be merged) until a review is submitted (a comment-only review by
# anyone, including the author, is fine);
# - if the check fails because of a review period, re-run it once the
# period is over.
#
# As with any pull_request_review workflow, this file is run from the pull
# request, so a pull request can change it. Therefore it is a governance file
# (see isGovernanceFile below), and changes to it must be reviewed carefully.
# Also, keep it read-only: do NOT add write permissions, secrets, or
# actions/checkout.
name: review-policy
on:
pull_request_review:
types: [submitted, edited, dismissed]

permissions: {}

jobs:
review-policy:
runs-on: ubuntu-26.04
permissions:
contents: read
pull-requests: read
steps:
- uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
with:
script: |
const { owner, repo } = context.repo;
const defaultBranch = context.payload.repository.default_branch;
// Review period for maintainer's pull requests with one LGTM.
const days = 7;
const largeDays = 14;
const largeLines = 100;

// MAINTAINERS lines look like "Name <email> (@login)". Always read
// it from the default branch, even for pull requests against
// release branches: this is the single source of truth.
const { data: file } = await github.rest.repos.getContent({
owner, repo, path: 'MAINTAINERS', ref: defaultBranch,
});
const maintainers = new Set(
Buffer.from(file.content, 'base64').toString()
.split('\n')
.map((line) => line.trim().split(' ').pop())
.filter((word) => word.startsWith('(@') && word.endsWith(')'))
.map((word) => word.slice(2, -1).toLowerCase()));
if (maintainers.size === 0) {
core.setFailed('No maintainers found in MAINTAINERS.');
return;
}
core.info(`Maintainers: ${[...maintainers].join(' ')}`);

// Use the current state rather than the event payload, which is stale
// when the job is re-run.
const { data: pr } = await github.rest.pulls.get({
owner, repo, pull_number: context.payload.pull_request.number,
});

// When the pull request was (last) marked as ready for review.
// A pull request opened as non-draft has no such event.
async function readySince(pr) {
// This returns all events (from all pages), oldest first.
const events = await github.paginate(github.rest.issues.listEvents, {
owner, repo, issue_number: pr.number, per_page: 100,
});
const ready = events.filter((e) => e.event === 'ready_for_review').pop();
return new Date(ready ? ready.created_at : pr.created_at);
}

// Files defining the project rules, or enforcing them.
function isGovernanceFile(f) {
return f === 'MAINTAINERS' || f === 'CODEOWNERS' ||
(f.endsWith('.md') && !f.includes('/') && f !== 'README.md') ||
f.startsWith('.github/workflows/review-policy');
}

// Documentation and CI configuration.
function isRoutineFile(f) {
return f.endsWith('.md') || f.startsWith('.github/');
}

// Returns 'governance', 'routine', or 'normal'.
function changeKind(pr, files) {
// The API returns at most 3000 files, so a governance file
// might be missing from the list.
if (files.length >= 3000) {
return 'governance';
}
const names = files.flatMap((f) =>
f.previous_filename ? [f.filename, f.previous_filename] : [f.filename]);
if (names.some(isGovernanceFile)) {
return 'governance';
}
if (pr.user.login === 'dependabot[bot]' || names.every(isRoutineFile)) {
return 'routine';
}
return 'normal';
}

async function evaluate(pr) {
if (pr.draft) {
return ['pending', 'Draft pull request.'];
}
const author = pr.user.login.toLowerCase();
const reviews = await github.paginate(github.rest.pulls.listReviews, {
owner, repo, pull_number: pr.number, per_page: 100,
});
// The latest non-comment review of each reviewer is what counts.
const latest = new Map();
for (const r of reviews) {
if (r.user && r.state !== 'COMMENTED' && r.state !== 'PENDING') {
latest.set(r.user.login.toLowerCase(), r.state);
}
}
const approved = [];
const blocked = [];
for (const [user, state] of latest) {
if (user === author || !maintainers.has(user)) {
continue;
}
if (state === 'APPROVED') {
approved.push(user);
} else if (state === 'CHANGES_REQUESTED') {
blocked.push(user);
}
}

if (blocked.length > 0) {
return ['failure', `Changes requested by ${blocked.join(', ')}.`];
}
if (approved.length >= 2) {
return ['success', `LGTMs from ${approved.join(', ')}.`];
}
const files = await github.paginate(github.rest.pulls.listFiles, {
owner, repo, pull_number: pr.number, per_page: 100,
});
const kind = changeKind(pr, files);
if (kind === 'routine') {
if (approved.length === 1) {
return ['success', `LGTM from ${approved[0]} (routine change).`];
}
return ['pending', 'Needs 1 maintainer LGTM (routine change).'];
}
if (kind === 'governance') {
return ['pending', `Needs 2 maintainer LGTMs (governance change), got ${approved.length}.`];
}
if (approved.length === 0 || !maintainers.has(author)) {
return ['pending', `Needs 2 maintainer LGTMs, got ${approved.length}.`];
}
const lines = files.reduce((sum, f) => sum + f.additions + f.deletions, 0);
const period = lines > largeLines ? largeDays : days;
const until = new Date((await readySince(pr)).getTime() + period * 86400 * 1000);
if (Date.now() >= until.getTime()) {
return ['success', `LGTM from ${approved[0]}, and ${period} days passed.`];
}
const date = until.toISOString().slice(0, 16).replace('T', ' ');
return ['pending', `Needs another LGTM, or re-run this check after ${date} UTC.`];
}

// Check runs belong to a commit, not to a pull request, so pull
// requests having the same head commit can't be told apart.
const open = await github.paginate(github.rest.pulls.list, {
owner, repo, state: 'open', per_page: 100,
});
const shared = open
.filter((p) => p.head.sha === pr.head.sha)
.map((p) => `#${p.number}`);
const [state, description] = shared.length > 1
? ['failure', `Same head commit in ${shared.join(', ')}; push a different commit to all but one.`]
: await evaluate(pr);
core.info(`#${pr.number} (${pr.head.sha}): ${state}: ${description}`);
await core.summary
.addRaw(`${description} See [the rules](${context.serverUrl}/${owner}/${repo}/blob/${defaultBranch}/MAINTAINERS_GUIDE.md#who-decides-what).`)
.write();
if (state !== 'success') {
core.setFailed(description);
}
34 changes: 30 additions & 4 deletions MAINTAINERS_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,37 @@ made through a pull request.

All decisions are pull requests, and the relevant maintainers make
decisions by accepting or refusing the pull request. Review and acceptance
by anyone is denoted by adding a comment in the pull request: `LGTM`.
by anyone is denoted by approving the pull request (an `LGTM`).
However, only currently listed `MAINTAINERS` are counted towards the required
two LGTMs. In addition, if a maintainer has created a pull request, they cannot
count toward the two LGTM rule (to ensure equal amounts of review for every pull
request, no matter who wrote it).
LGTMs, and the pull request author's own LGTM is never counted.

A pull request can be merged once it has:

* two LGTMs from maintainers; or
* one LGTM from a maintainer, if it is a routine change; or
* one LGTM from another maintainer, if the author is a maintainer, the pull
request is not a governance change, and it has been open for review (that
is, not a draft) for at least 7 days, or 14 days if it is a large change
(more than 100 lines added and removed). This gives other maintainers a
chance to review and object.

A pull request is a governance change if it changes the rules of the project,
or how they are enforced. That is, it changes `MAINTAINERS`, `CODEOWNERS`,
a top-level `*.md` file other than `README.md` (such as this document), or the
`review-policy` workflow. A governance change always requires two LGTMs.

A pull request is a routine change if it is not a governance change, and it is
either:

* opened by Dependabot; or
* only changes documentation (`*.md` files) and/or CI configuration (files
under `.github/`).

In all cases, a maintainer requesting changes blocks the merge until the
request is resolved (the reviewer approves, or the review is dismissed).

These rules are enforced by the
[`review-policy`](.github/workflows/review-policy.yml) check.

Overall the maintainer system works because of mutual respect.
The maintainers trust one another to act in the best interests of the project.
Expand Down
Loading