Gate PRs on the full test suite, not just after merge - #6699
Draft
dayaffe wants to merge 17 commits into
Draft
Conversation
coverage.yml runs make test-all (all-features + failpoints + every broker backend) only on push to main, so broken optional features or broker integrations land on main before anyone notices. Add a pull_request-triggered workflow that runs the same suite before merge.
apt's protobuf-compiler doesn't support proto3 optional fields by default, which the substrait crate (pulled in by --all-features via the datafusion feature) requires. coverage.yml already works around this the same way; this job needs it too since it now builds with --all-features.
Running the full broker/all-features suite unconditionally on every PR push doesn't match how the team already runs CI on the sibling vector repo: there, the equivalent full suite is merge-queue- or maintainer-comment-gated, never automatic-for-everyone. full-tests.yml now only runs via workflow_dispatch. The new full-tests-trigger.yml fires on a submitted PR review whose body starts with /ci-run-full-tests, checks the reviewer's actual repos.getCollaboratorPermissionLevel (maintain/admin — not author_association, which can't distinguish write-only collaborators from maintainers), sets a pending commit status, and dispatches the run against the reviewed commit. It's still a required status check: since only maintain/admin reviewers can trigger it and that's also who can merge, a PR can't merge without that same person having run and passed the full suite themselves.
full-tests.yml: - Split status-posting into its own jobs (set-pending, report-status) so the job that checks out and runs untrusted PR content (full-tests) never holds statuses:write — previously that job could have forged a passing status onto any commit via the token persisted by actions/checkout during `make test-all`. - persist-credentials: false on that checkout, since it doesn't need git credentials at all. - Validate inputs.sha/inputs.pr_number to a strict shape (validate-inputs) before any other job trusts them, and pass them to github-script via env:/process.env rather than templating into script source — workflow_dispatch inputs are free-text fields anyone with repo write access can set to anything via the Actions UI/API, so a crafted value could otherwise break out of a string literal in a step holding statuses:write. full-tests-trigger.yml: - Bind the permission check to github.event.review.user.login (who actually submitted the review) instead of context.actor (whoever triggered this workflow run) — those differ on a manual re-run, where actor becomes the re-runner while the review payload stays frozen from the original event.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
|
/ci-run-full-tests |
pull_request_review requires using Files changed -> Review changes -> Submit review with the command as the review body; a normal comment on the Conversation tab (issue_comment) doesn't fire it, which is exactly how the first real attempt to use this failed. issue_comment has no commit sha the way a submitted review does, so this resolves the PR's current head sha itself via pulls.get before posting status or dispatching.
Only running the redesigned validate-inputs/set-pending/full-tests/ report-status split, the protoc fix, and the docker-compose service setup once so far, on a maintainer-comment trigger that can't fire until this merges to main. Adding pull_request: here to actually see it pass before switching the real trigger on. Doesn't post commit statuses on pull_request runs (gated to workflow_dispatch only) so this can't accidentally satisfy the same-named required check on other PRs in the repo. Remove this trigger and the two workflow_ dispatch-only conditions before merging.
gh-ubuntu-arm64 killed the job at ~8 minutes on two separate runs (7m45s and 7m35s), both mid-compile, both with a runner shutdown signal rather than a test failure or our own timeout-minutes. Matches how vector's CI (test.yml) runs everything on plain GitHub-hosted runners (ubuntu-24.04, ubuntu-24.04-8core for the heavy jobs) rather than a custom label. Starting with the standard tier since we can't confirm quickwit-oss has GitHub's larger-runner tier provisioned.
- full-tests-trigger.yml now listens on pull_request_review (submitted) and uses the immutable review.commit_id, so the suite can't drift to a later push. - full-tests.yml is now a reusable workflow (workflow_call) also triggered by merge_group, so merge-queue candidates run the full suite automatically. - validate-inputs distinguishes the review path (PR open + head matches the reviewed SHA) from the merge-group path. - Failure notification labels the target as PR #N or merge-group commit. - Use standard ubuntu-24.04 runners.
CodeQL's actions/untrusted-checkout rule flags the checkout in the full-tests job. The construct is intentional and already mitigated by privilege isolation, so record the reasoning in-place rather than restructuring the job: no secrets reference, contents: read only, persist-credentials: false, and a ref that is always either a maintainer-reviewed commit or a merge-queue candidate.
Switch the trigger from pull_request_review to issue_comment. Fork PR review runs get a read-only token with secrets withheld, so the status writes would 403 and the required check could never be satisfied for external contributors -- the people this gate most needs to cover. issue_comment is a default-branch event, so the workflow definition is never read from the PR and the run keeps the scopes it needs. Test the PR's merge commit rather than its head, so the result reflects the code as merged. The command carries no SHA and means 'test this PR now'; both SHAs are resolved once, up front, and the run is pinned to that snapshot. Poll until GitHub has computed mergeability, since merge_commit_sha is populated asynchronously and would otherwise point at a stale tree. Make the untrusted job's cache read-only. Because the caller is a default-branch event, a cache written by PR-authored code would land in the default-branch scope, where every other PR and every trusted workflow on main would read it. Also simplify: fold SHA resolution into the pending-status job (dropping validate-inputs and its now-dead input validation, since SHAs come from the API rather than from a comment), and stop re-declaring workflow-level env inside steps. Rename the status context to full-test-suite: it is a long-lived branch protection contract and should not carry a Makefile target name.
Comment on lines
+126
to
+131
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| ref: ${{ needs.prepare.outputs.test_sha }} | ||
| persist-credentials: false | ||
|
|
||
| - name: Install Ubuntu packages |
Also fixes a real bug in the authorization check: getCollaboratorPermissionLevel's `permission` field is coarse and reports a `maintain` role as `write`, so comparing it against 'maintain' admitted admins only. Threshold is now write access or above, stated explicitly.
Three changes: - Report the status on the PR head only, not on both the head and the merge commit. Every other required check here is a check run on the head, so that is where branch protection looks; the merge-commit status was ignored for gating and only produced a duplicate entry in the PR check list. The tested merge commit is now named in the status description instead. - Read the `quickwit-cargo` cache rather than a dedicated `quickwit-cargo-full` key. With `save-if: false` nothing would ever have written a private key, so every run would have started cold. ci.yml populates `quickwit-cargo` from pushes to main on the same x64 runners (cache keys are arch-scoped) and its lints job builds --all-features. - Drop the temporary pull_request trigger and its fallbacks now that the job graph is proven: 3022 tests + 10 failpoints tests green.
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
2 participants
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
make test-allcurrently runs after merge throughcoverage.yml. It exercises all features, failpoints, and the broker backends, while PR CI runscargo nextestwith only thepostgresandmetricsfeatures. Regressions in optional features or broker integrations therefore reachmainbefore anyone sees them.This PR adds a
full-test-suitecommit status with two entry points:/ci-run-all-testson a PR. The trigger verifies the commenter through the GitHub API, resolves the PR head and test merge commit, and polls until GitHub has computed mergeability. Conflicting PRs fail before the suite starts.merge_groupruns the same workflow for merge-queue candidates.For PR comments, the workflow tests the PR's merge commit and reports the status on the PR head, alongside the repository's existing required checks. A subsequent push creates a new head without the status and requires the suite to run again. Both entry points publish the same
full-test-suitecontext.The comment trigger uses
issue_commentso it can publish statuses for fork PRs;pull_request_reviewreceives a read-only token for forks. Execution of PR code is isolated in a job withcontents: read, no secrets,persist-credentials: false, and a read-only cache. Status-writing jobs do not check out PR code, the notification job has no repository permissions, authorization uses the comment author from the event payload, and third-party actions are SHA-pinned. CodeQL'sactions/untrusted-checkoutalert is mitigated by these controls and documented inline.The suite takes approximately 22 minutes and has a 60-minute timeout.
Rollout will be staged: merge without requiring
full-test-suite, verify the command on a same-repository PR and a fork PR, then add the status to themainruleset. The authorization, fork-PR, andmerge_grouppaths require the workflow to be present on the default branch and have not yet been exercised. Enabling a merge queue will also requiremerge_grouptriggers on the existing required checks.Follow-up: update
CONTRIBUTING.md, which still describes the exhaustive suite as post-merge only.How was this PR tested?
Full test suite run 32291436144, via a temporary
pull_requesttrigger that has since been removed:full-test-suitestatuses were published successfully