Repository navigation
Conversation
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>
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. This is a copy of the runc check (see opencontainers/runc#5512), adapted for this repo (no vendor/ and CHANGELOG.md, but CODEOWNERS). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the same as opencontainers/runc#5512 (see there for the rationale
and discussion), with the only differences being local specifics:
vendor/here, so all changed lines count towards thelarge change threshold;
CHANGELOG.md, soREADME.mdis the only top-level*.mdfile which is not a governance file;
CODEOWNERSis added to governance files;MAINTAINERS_GUIDE.mdwording is merged with the new text.The follow-up opencontainers/runc#5563 (
merge_groupsupport) is notneeded here, as this repo does not use the merge queue.
After merging
A repo admin needs to update the
mainbranch protection:review-policy(from GitHub Actions) as a required status check.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.