Gate PRs on the full test suite, not just after merge - #6699
Draft
dayaffe wants to merge 8 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.
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.
Adds a required full-test-suite check (
make test-all: all-features + failpoints + every broker backend — Kafka, Pulsar, Azurite, fake-GCS, Pub/Sub emulator, LocalStack, Postgres). Today that suite only runs post-merge viacoverage.yml;ci.ymlonly exercises 2 of ~15 feature flags on PRs, so a broken optional feature or broker integration can land onmainbefore anyone notices.full-tests.ymlruns only viaworkflow_dispatch, triggered byfull-tests-trigger.ymlwhen a PR comment starting with/ci-run-full-testsis posted by someone holdingmaintain/adminpermission on the repo (checked via the API, not just comment/review association).full-tests.ymlalso has a temporarypull_request:trigger active right now (clearly marked in the file) to confirm the job graph passes on this PR before merge; it will be removed before this goes in.