Repository navigation
ci: run the suite for pull requests into any branch - #190
Conversation
|
The red on 3.11/3.12/3.13 is master's, not this change's: the failing tests are So this PR cannot be green until #186 lands. Rebase it onto master afterwards and it should come up clean; nothing in it depends on the toon work. The useful side effect: with this merged, a stacked PR whose base is at or ahead of it gets a normal run, so #185 will stop needing |
A pull request stacked on another branch matched no trigger: the `pull_request` filter was limited to `branches: [master]`, so the run never existed. Absent checks read as "not started" rather than "never ran", and a stacked head could only be verified with `gh workflow run ci.yml --ref <branch>` by hand. Stacked PRs are a supported workflow here, and a stacked PR costs the same matrix as any other, so the filter is dropped rather than documented around. `push` stays master-only; the pull request trigger covers feature branches.
5989936 to
bed3a47
Compare
|
Rebased onto master as The red it carried was master's pre-toon-fix drift, exactly as described above — Ready to land. With it in, a stacked PR whose base is at or ahead of this gets a normal run instead of needing |
There was a problem hiding this comment.
Verified independently at bed3a47, and the premise holds. I reconstructed it from the repo's own history rather than taking the probe on trust.
The change. Exactly one file, +6/-1. pull_request.branches: [master] is dropped and replaced by the comment explaining why; parsing the file at the head, pull_request: is bare, so it takes all activity types and all base branches. push stays master-only, which is the right half to leave alone — widening that too would add a second run to every push to a feature branch that already has a PR. workflow_dispatch is retained, so the transition path stays available. I also checked the other workflows: lint-test is workflow_call, and benchmark/pypi/release trigger on release or dispatch, so ci.yml is the only one with a pull_request trigger and nothing else gains runs from this.
The premise, independently. #185's base changed twice, from the timeline:
08:22:40— master ->fix/toon-spec-4.109:35:54—fix/toon-spec-4.1-> master
Between those two, 73 minutes on a non-master base, the only run on that branch is the workflow_dispatch at 08:33:56. The moment the base changes back to master, a pull_request run appears at 09:37:10. That is the narrow filter failing exactly where you say it fails and clearing exactly when the base matches — and it does not depend on reading the manual dispatch as its own explanation.
The other half. Your probe is the complement: run 38374799401 is a genuine pull_request run on tmp/stacked-trigger-probe, and PR #189's base was fix/toon-spec-4.1 — a non-master base that produced a run because the head carried the widening. Both directions are evidenced now. #189 closed at 09:14:38 and the probe branch is gone from origin.
Transition caveat, confirmed. A stacked head whose base predates this still needs workflow_dispatch, which matches what #185 needed. Agreed it is transitional rather than steady state, since new stacks branch from master and inherit the change.
Non-blocking, no action required:
- The branch's merge-base is
6ac319a, so the green checks ran on a tree predating #182. One workflow file cannot change a test outcome, so this is a formality; a rebase before merge would put the green run on the merged tree if you want that on the record. - Scope: with the filter gone, PRs into any base match, including the docs orphan branches (
docs_en,docs_zh) andgh-pages, where the Python matrix has nothing to say. Docs are committed straight from the worktrees here, so it is unlikely to come up;branches-ignorewould scope it if you would rather not rely on that. - True after this lands and outside its scope: a stacked PR's checks do not re-run when its base advances, since no event fires for the dependent PR, so a stacked head can show green from a stale merge commit. Worth knowing about, not worth solving here.
CI is green (lint plus 3.10 through 3.13) and the change does what it claims.
There was a problem hiding this comment.
Reviewed against the merge base 6ac319a, so the diff is the change itself and not the stale-base noise that a plain master..branch diff shows for this branch: five comment lines added and one filter line removed.
The mechanism is right. A pull_request branches filter matches the PR's base branch, so with branches: [master] a PR stacked on another feature branch matched no trigger at all. Verified on the file as it will be read: pull_request now parses to a present-but-unfiltered trigger, push keeps branches: [master], and the ci job set is unchanged.
What this is honest about, and what I could not verify. This PR's own run is master-based — lint plus 3.10-3.13, all green — so its green checks are not evidence for the case being fixed. No open PR currently targets a non-master base either (#196, #195, #194, #193, #190, #171 are all master), so the stacked case cannot be observed until the next stacked pair exists. That is inherent to a trigger filter, not a gap in the change, but it does mean the fix is unverified by its own CI: the thing to do is confirm the next stacked PR shows checks on its own head without gh workflow run. The comment in the file says exactly that this was the previous workaround, so it will be obvious if it did not take.
No duplicate-trigger risk. push: branches: [master] is untouched, so a merge to master still yields one push run, and pull_request adds one run per PR. The added cost is one matrix per stacked PR, which the comment states and which is the intended trade — submit-workflow.md documents stacking as supported, so CI now matches the documented workflow instead of quietly contradicting it.
Approving. Small, correct, and it removes the "no checks reads as not started" trap for good.
What
ci.ymllimited itspull_requesttrigger tobranches: [master], so a pull request stacked on another branch matched no trigger and no run was ever created. Absent checks read as "not started" rather than "never ran", and a stacked head could only be verified by hand:That is how #185 (stacked on #186) had to be verified —
gh pr checks 185showed nothing but GitGuardian while all four matrix jobs were fine.The filter is dropped.
pushstays master-only; the pull request trigger now covers any base.Why not just document the manual path
Stacked PRs are a supported workflow here —
submit-workflow.mdhas a section on them — and the point of a stacked head is to know whether it is green before the base merges. A trigger that silently produces no checks works against that, and the failure is invisible in the direction that matters: nothing on the PR says the workflow was skipped.The cost is the same matrix a normal PR already runs. Nothing double-runs: a stacked PR runs once, for itself.
Verified
The trigger is evaluated from the PR's merge commit, not from the base branch, so a stacked head picks this up as soon as the head carries the file. Probe: a throwaway branch carrying exactly this change, based on
fix/toon-spec-4.1, opened as #189, produced apull_requestrun (38374799401) — where the same base without the change produced no run at all. Probe closed and its branch deleted.So stacked PRs get normal CI once their base is at or ahead of this change. A stacked head whose base predates it still needs
workflow_dispatch, which is a transition rather than the steady state.