Skip to content

ci: dispatch coverage-fanout on merged source PRs#459

Open
eric-wang-1990 wants to merge 6 commits into
mainfrom
eric-wang-1990/ci/coverage-fanout-sender
Open

ci: dispatch coverage-fanout on merged source PRs#459
eric-wang-1990 wants to merge 6 commits into
mainfrom
eric-wang-1990/ci/coverage-fanout-sender

Conversation

@eric-wang-1990

Copy link
Copy Markdown

Summary

Wires databricks-sql-nodejs into the multi-language coverage fan-out in databricks/databricks-driver-test. When a PR merges to main and touched driver source (a file under lib/), dispatch a coverage-fanout repository_dispatch to driver-test; its coverage-fanout-tracker.yml opens 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/databricks already runs; this is one of a set of sibling PRs bringing the remaining driver repos onto the flow.

What it does

  • Adds closed to the pull_request trigger types; the new trigger-coverage-fanout job gates on github.event.pull_request.merged == true.
  • Source-path filter (lib/): docs/CI/test-only merges don't kick off a full 7-leg fan-out.
  • Reuses the existing INTEGRATION_TEST_APP_ID/_PRIVATE_KEY App token (scoped to driver-test) + the same peter-evans/repository-dispatch pin adbc uses.
  • Tightens skip-integration-tests-pr's guard to exclude closed so it doesn't re-stamp a check on merged PRs.

Test Plan

  • YAML validates; job-guard audit confirms no existing job misfires on the new closed event.
  • After merge: a subsequent merged source PR shows a coverage-fanout dispatch + a new tracking issue in databricks/databricks-driver-test.

This pull request and its description were written by Isaac.

…ce PRs

Wires databricks-sql-nodejs into the multi-language coverage fan-out. When a PR merges to
main and touched driver source (a file under lib/), 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 (lib/): 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>
Copilot AI review requested due to automatic review settings July 23, 2026 20:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…ermissions

peco-review-bot findings on the coverage-fanout sender (apply to all driver
repos — the job is identical everywhere):

- F1 (Medium): the merged-PR guard didn't constrain the base branch, so a PR
  merged into a release/feature branch that touched source would also dispatch
  a full fan-out authoring a spec from a diff that never reached main. Add
  `github.event.pull_request.base.ref == 'main'` to match the stated intent.
- F2 (Low): the job declared no permissions block, relying on the default
  GITHUB_TOKEN read scope for github.rest.pulls.listFiles; if org defaults
  tighten to none it 403s silently. Scope it explicitly: contents: read +
  pull-requests: read.

Co-authored-by: Isaac
Signed-off-by: Eric Wang <e.wang@databricks.com>
Further peco-review-bot findings on the coverage-fanout sender:

- Narrow the minted App installation token with `permission-contents: write`
  (all coverage_fanout needs is repository_dispatch → contents:write), matching
  the defense-in-depth the other dispatch jobs in these repos already use — so a
  leaked token can only fire dispatches, not exercise the App's full scope.
- Restore the version tag in two action-pin comments (`# pinned` → the exact
  `# vX.Y.Z` the SHA corresponds to, per repo convention) for auditability.

Co-authored-by: Isaac
Signed-off-by: Eric Wang <e.wang@databricks.com>
Send proxy_mode=replay in both dispatch payloads (the label preview and the
merge-queue required gate) so the databricks-sql-nodejs PR gate runs the Node.js
suite in REPLAY against the PR's driver commit — deterministic, credential-free,
no live warehouse — instead of the live passthrough run it does today.

Paired with the driver-test receiver change (databricks-driver-test#909) that
adds proxy_mode=replay to databricks-sql-nodejs-integration-tests.yml and skips
the recording-less reyden leg in replay. Both must merge for the gate to run
replay; until #909 lands the receiver ignores proxy_mode (stays passthrough).

Co-authored-by: Isaac
Signed-off-by: Eric Wang <e.wang@databricks.com>
@eric-wang-1990 eric-wang-1990 added the integration-test Trigger the cross-repo driver-test Node.js integration suite on this PR label Jul 24, 2026
@github-actions

Copy link
Copy Markdown

Node.js integration tests triggered. View workflow run.

@eric-wang-1990 eric-wang-1990 removed the integration-test Trigger the cross-repo driver-test Node.js integration suite on this PR label Jul 24, 2026
@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@eric-wang-1990 eric-wang-1990 added the integration-test Trigger the cross-repo driver-test Node.js integration suite on this PR label Jul 24, 2026
@github-actions

Copy link
Copy Markdown

Node.js integration tests triggered. View workflow runs. The result posts back here as the "Node.js Integration Tests" check.

The "Node.js integration tests triggered" PR comment linked to
databricks-nodejs-integration-tests.yml, which does not exist (404) — the
receiver file is databricks-sql-nodejs-integration-tests.yml. Point to the
correct workflow and note that the authoritative result is the "Node.js
Integration Tests" check posted back on the PR.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
@eric-wang-1990
eric-wang-1990 force-pushed the eric-wang-1990/ci/coverage-fanout-sender branch from dabdb86 to 15ab3eb Compare July 24, 2026 07:22
@github-actions github-actions Bot removed the integration-test Trigger the cross-repo driver-test Node.js integration suite on this PR label Jul 24, 2026
@github-actions

Copy link
Copy Markdown

Integration test approval reset.

New commits were pushed to this PR. The integration-test label has been automatically removed for security.

A maintainer must re-review the changes and re-add the label to trigger tests again.

Latest commit: 15ab3eb

@eric-wang-1990 eric-wang-1990 added the integration-test Trigger the cross-repo driver-test Node.js integration suite on this PR label Jul 24, 2026
@github-actions

Copy link
Copy Markdown

Node.js integration tests triggered. View workflow runs. The result posts back here as the "Node.js Integration Tests" check.

sreekanth-db
sreekanth-db previously approved these changes Jul 24, 2026

@sreekanth-db sreekanth-db left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[withdrawn]

@eric-wang-1990
eric-wang-1990 added this pull request to the merge queue Jul 24, 2026
@sreekanth-db
sreekanth-db removed this pull request from the merge queue due to a manual request Jul 24, 2026
@sreekanth-db
sreekanth-db dismissed their stale review July 24, 2026 10:11

Dismissing approval.

@sreekanth-db

Copy link
Copy Markdown
Contributor

🟡 [MAJOR] Source filter too narrow — misses driver source outside lib/

The isSource filter only matches files under lib/, but driver source also lives at the repo root: KERNEL_REV, native/ (kernel bindings), and thrift/ (Thrift defs). This same workflow (lines 135-139, 228-232) documents that path-gating on a file list previously skipped these Thrift-affecting changes. A merged PR that only bumps KERNEL_REV or edits native/ or thrift/ will report srcChanged=false and never dispatch coverage-fanout.

Suggested fix — broaden the source predicate to include the other driver-source roots:

            const isSource = (f) => f.startsWith('lib/') || f.startsWith('native/') || f.startsWith('thrift/') || f === 'KERNEL_REV';

(via Isaac Review)

The isSource predicate only matched lib/, but driver source also lives at
native/ (kernel bindings), thrift/ (Thrift defs), and the KERNEL_REV pin.
A merged PR that only bumped KERNEL_REV or edited native/ or thrift/ would
report srcChanged=false and never dispatch coverage-fanout, even though it
affects the Thrift/kernel backends — the same gap the "run on every change"
comments in this workflow already document. Include those roots.

Co-authored-by: Isaac
Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
@github-actions github-actions Bot removed the integration-test Trigger the cross-repo driver-test Node.js integration suite on this PR label Jul 24, 2026
@github-actions

Copy link
Copy Markdown

Integration test approval reset.

New commits were pushed to this PR. The integration-test label has been automatically removed for security.

A maintainer must re-review the changes and re-add the label to trigger tests again.

Latest commit: 6fdff86

@eric-wang-1990

Copy link
Copy Markdown
Author

Fixed in 6fdff86 — broadened isSource to lib/ + native/ + thrift/ + KERNEL_REV (verified all four roots exist in the repo) and updated the comment to match the source roots the "run on every change" blocks already document. Thanks for the catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants