ci: run tests and linters on pull requests to any base branch - #299
Open
mlwelles wants to merge 2 commits into
Open
ci: run tests and linters on pull requests to any base branch#299mlwelles wants to merge 2 commits into
mlwelles wants to merge 2 commits into
Conversation
ci-dgraph4j-tests.yml and trunk.yml both filtered their pull_request trigger to base branch main, so a pull request targeting a feature branch ran neither the test suite nor the linters. Only CodeQL ran, because it comes from GitHub default setup rather than a workflow in this repo. That combination is worse than no checks. GitHub reports such a pull request mergeable and CLEAN, because checks that are absent cannot fail, so it reads as validated while nothing has validated it. The gap matters for stacked pull requests, where every entry above the bottom targets the branch below it. GitHub stack merge lands a stack atomically, which is its main draw and which merges those upper entries without the suite ever running on them. A problem would surface only in the post-merge push run against main, after the fact. Drop the branch filter from pull_request in both workflows rather than listing branch prefixes. A pull request deserves the same checks wherever it is aimed, and an allowlist would need editing for every new naming convention. push stays limited to main, so no feature-branch push triggers a build on its own.
Widening the pull_request trigger means the Dgraph-from-source build now runs on stacked pull requests too, and a chain of N runs it N times. Every push to a branch under review also starts a fresh run while the previous one is still going. Cancel the in-flight run when a pull request is pushed again. The group key uses head_ref for pull requests and falls back to ref elsewhere, and cancel-in-progress is gated on the event being a pull_request, so a main-branch push or the nightly schedule is never cancelled -- those are the runs whose results are worth keeping even when superseded.
3 tasks
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
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.
Description
ci-dgraph4j-tests.ymlandtrunk.ymlboth filtered theirpull_requesttrigger to base branchmain, excluding any pull request aimed at a feature branch. Only CodeQL ran on one, since it comes from GitHub's default setup rather than a workflow in this repo.That combination is worse than having no checks at all. GitHub reports such a pull request
mergeable: MERGEABLEandmergeStateStatus: CLEAN, because checks that are absent cannot fail — so it reads as validated while nothing has validated it. Measured on #297 ("fix: report a missing refresh token as AuthException"), which is based on a feature branch: 4 checks present against 7 on a pull request based onmain, withdgraph4j-tests,Trunk Check, andTrunk Code Quality / Checkall missing.Why native stacked PRs do not make this redundant
Native stacks cover part of the gap on their own: #297 now runs
dgraph4j-testsdespite its base beingfix/async-client-non-blocking-retries. The linters still do not run there — noTrunk Check, noTrunk Code Quality / Check— so a stacked pull request is tested but unlinted. This PR closes that half.It also stops coverage depending on inferred behavior. Nothing documents why the suite now runs on a non-
mainbase while the linters do not, and an explicit trigger outlasts that asymmetry.gh stack mergelands a whole stack atomically, so every entry wants its full checks before that happens, not after.Two commits, reviewable separately
ci: run tests and linters on pull requests to any base branch— drops thebranchesfilter frompull_requestin both workflows. Dropping it beats listing prefixes such asfix/**: a pull request deserves the same checks wherever it is aimed, and an allowlist needs editing for every new naming convention.pushstays limited tomain, so no feature-branch push triggers a build on its own, and the nightlyscheduleis untouched.ci: cancel superseded pull request runs— the cost offset, and droppable on its own if you would rather not take it. Widening the trigger means the Dgraph-from-source build runs on stacked pull requests too, and a chain of N runs it N times. This cancels the in-flight run when a pull request is pushed again.cancel-in-progressis gated on the event being apull_request, so amainpush or the nightly schedule is never cancelled.Verification
pushstill{branches: [main]},pull_requestkeeps its four activity types with no branch filter,scheduleunchanged.trunk check .github/workflows/reports no issues, actionlint included.main, so it cannot demonstrate the fix on itself. The check count on fix: report a missing refresh token as AuthException #297 rising from 4 to 7 after this merges is the observable outcome.No source or public API changes. Per this repo's precedent for CI-only changes —
chore (ci): add stale action(#291) has no entry — there is noCHANGELOG.mdaddition.Checklist
CHANGELOG.mdfile describing and linking tothis PR — n/a, CI configuration only, matching the precedent above
n/a, no behavior to test; the effect is observable as check coverage on stacked pull requests
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.