Replies: 4 comments 3 replies
|
#3460 is merged, so The CI half needs an Infra ticket, and one correction to my post above: I brushed off runner time, and runner time is the entire concern. That policy grants the "only require approval first time" option to projects that open a ticket showing a list discussion where members commit to "actively monitor their workflows for abuse" and act on it. There is no What I would commit us to: fork runs are public at https://github.com/apache/maka/actions and pull request activity already mails commits@; any fork run that is not plainly the PR's own tests gets cancelled by whoever sees it first; on any abuse, I ask Infra to restore the default rather than argue. PPMC members: if you agree, please reply +1 — a reaction on the post is not something the ticket can cite. Mentors, an acknowledgement would help. I will hold the ticket a few days. |
|
Strongly agree |
|
+1! |
|
+1 (binding). |
Uh oh!
There was an error while loading. Please reload this page.
Two settings currently slow every pull request down for no safety we actually get. I would like to change both, and I would rather find out they were wrong by reverting them than by debating them for a week. Concretely: I intend to merge the pull request below as soon as another committer approves it, and to revert it immediately if anyone here objects afterwards. Say so on this thread and it goes back. If you would rather decide before it lands, say that instead and I will hold it.
1. Approvals should survive a push
Today
.asf.yamlsetsdismiss_stale_reviews: trueonmain. GitHub dismisses an approval on any new commit, but it never dismisses a requested change: an objection stays until the reviewer withdraws it, while an approval is thrown away by a rebase ontomainor a one-line fixup. The two halves of the same review decay at different rates, and the cost is worst on the pull requests that need review most, because the longer one stays open the more often it is rebased.GitHub offers no rebase-aware option. The flag dismisses on every commit or on none. I propose none.
What we keep:
required_status_checksrerunstestagainst every pushed head, so the mechanical guarantee is still head-bound. What we give up is GitHub asserting that an approval refers to the exact tree it was given on. That was never a strong claim — an approving committer could already push and self-merge — and the honest control for it is a reviewer asking for another look.CONTRIBUTING.mdwill say so: push follow-up commits in the open, and request another review when a change outgrows what was approved.Pull request: #3460
2. CI should run automatically for contributors we have seen before
Workflow runs on pull requests currently need a maintainer to press Approve every time, for every contributor. In practice a new contributor's first signal arrives whenever someone happens to be looking, and returning contributors keep paying the same toll on every push.
I propose the standard GitHub policy instead: require approval for first-time contributors. Someone whose first pull request has never been merged here still gets a human gate before their workflow touches our runners, which is the case the setting exists for. Everyone past that point gets CI immediately.
The risk this guards against is a fork pull request running our workflows. Our
ci.ymltriggers onpull_request, notpull_request_target, so a fork's code runs without our secrets and withcontents: read; release credentials sit behind thereleaseandnpm-releaseenvironments with a named human reviewer. Beyond that, what a per-run approval protects is runner time..asf.yamlhas no key for this one, so it needs an INFRA ticket rather than a commit, and INFRA will want to see this thread. I will file it once nobody here objects.Why this way
Both are one-line settings changes with no migration and no data behind them, so reverting either is immediate and costs nothing. That is why I am proposing to land the first one and listen, rather than hold it. The second cannot work that way — an INFRA ticket is not something I can undo by pushing a commit — so that half waits for this thread either way.
All reactions