ci: dispatch coverage-fanout on merged source PRs#879
Conversation
…ce PRs Wires databricks-sql-python into the multi-language coverage fan-out. When a PR merges to main and touched driver source (a file under src/), dispatch a `coverage-fanout` repository_dispatch to databricks/databricks-driver-test. Its coverage-fanout-tracker.yml then opens a tracking issue and runs the language-agnostic fan-out (a spec authored from this PR's diff, conformed across every driver). - Adds `closed` to the pull_request trigger types; the new trigger-coverage-fanout job gates on pull_request.merged == true. - Source-path filter (src/): docs/CI/test-only merges don't warrant a full fan-out. - Reuses the existing INTEGRATION_TEST App token (scoped to driver-test) + the same peter-evans/repository-dispatch pin adbc-drivers/databricks uses. - Tightens skip-integration-tests-pr's guard to exclude `closed` so it doesn't re-stamp a check on merged PRs. Co-authored-by: Isaac Signed-off-by: Eric Wang <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-scoped CI addition that mirrors the existing dispatch pattern. One low-severity note: the new trigger-coverage-fanout job omits an explicit permissions: block that every sibling job in this file declares, so its pulls.listFiles call relies on the org's default GITHUB_TOKEN permissions. The closed-event guards and skip-integration-tests-pr exclusion are correct, and the client-payload has no injection surface (no PR-title interpolation).
Addresses: - #3641231149 at .github/workflows/trigger-integration-tests.yml:468 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good overall — the closed event is added cleanly and all existing jobs have explicit event/action guards, so none misfire on it (and skip-integration-tests-pr correctly excludes closed). One medium concern: the new trigger-coverage-fanout job has no base-branch guard, so it fires for merges into any branch, not just main as the description intends.
Addresses: - #3641618444 at .github/workflows/trigger-integration-tests.yml:459 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Addresses: - #3641913695 at .github/workflows/trigger-integration-tests.yml:513 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the new trigger-coverage-fanout job is well-gated (merged + base.ref==main + src/ path filter), the skip-integration-tests-pr guard is correctly tightened to exclude closed, and I audited every other job's if (synchronize / labeled / merge_group) to confirm none misfires on the new closed event. One low note on the failure-handler's comment wording. Nit: this job declares pull-requests: write but not issues: write, whereas the sibling comment-posting jobs declare both; issues.createComment on a PR works with pull-requests: write and the call is wrapped in try/catch, so this is non-fatal, but the permission set is inconsistent with the rest of the file.
Addresses: - #3641939652 at .github/workflows/trigger-integration-tests.yml:530 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the closed-event guard audit checks out: every pre-existing job correctly ignores closed (synchronize/labeled/merge_group), skip-integration-tests-pr now excludes it as intended, and the new trigger-coverage-fanout job is properly gated on merged == true + base.ref == 'main'. Payload interpolations are injection-safe and the failure-handler gate reasoning is sound. One low-severity note about duplicate dispatch on manual re-runs.
Addresses: - #3641958020 at .github/workflows/trigger-integration-tests.yml:540 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid — the closed event is correctly fenced off from every pre-existing job, and the new trigger-coverage-fanout job's gating/failure-handler reasoning is sound. One medium concern: for merged fork PRs, pull_request events don't carry secrets, so the fan-out (and its failure comment) can silently no-op.
Addresses: - #3641977677 at .github/workflows/trigger-integration-tests.yml:494 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall — the closed-event wiring is well-guarded (every other job correctly excludes the new event) and the failure-handler/truncation reasoning is sound. One medium concern: fork src merges aren't cleanly excluded at the job level, so they produce a red workflow run rather than the silent exclusion the header comment claims.
Addresses: - #3641995560 at .github/workflows/trigger-integration-tests.yml:476 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a carefully-scoped CI change with thorough gating and inline rationale. I verified the whole workflow: on the new closed/merged event no existing job misfires (remove-label-on-new-commit gates on synchronize, skip-integration-tests-pr now excludes closed, trigger-tests-pr on labeled, merge-queue-python on merge_group), and the new trigger-coverage-fanout job correctly requires merged==true + base main + non-fork, serializes via concurrency, and fails safe on the source-detection path. One low-severity permissions observation noted inline.
Addresses: - #3642007200 at .github/workflows/trigger-integration-tests.yml:493 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Summary
Wires databricks-sql-python into the multi-language coverage fan-out in
databricks/databricks-driver-test. When a PR merges tomainand touched driver source (a file undersrc/), dispatch acoverage-fanoutrepository_dispatchto driver-test; itscoverage-fanout-tracker.ymlopens a tracking issue and runs the language-agnostic fan-out — a spec authored from this PR's diff, conformed as tests across every driver (csharp/python/go/nodejs/rust/kernel/jdbc).Same sender
adbc-drivers/databricksalready runs; this is one of a set of sibling PRs bringing the remaining driver repos onto the flow.What it does
closedto thepull_requesttrigger types; the newtrigger-coverage-fanoutjob gates ongithub.event.pull_request.merged == true.src/): docs/CI/test-only merges don't kick off a full 7-leg fan-out.INTEGRATION_TEST_APP_ID/_PRIVATE_KEYApp token (scoped to driver-test) + the samepeter-evans/repository-dispatchpin adbc uses.skip-integration-tests-pr's guard to excludeclosedso it doesn't re-stamp a check on merged PRs.Test Plan
closedevent.coverage-fanoutdispatch + a new tracking issue indatabricks/databricks-driver-test.This pull request and its description were written by Isaac.