Skip to content

ci: add label-gated proxy-test dispatch to databricks-driver-test - #799

Merged
vikrantpuppala merged 1 commit into
mainfrom
feat/python-proxy-test-dispatch
May 22, 2026
Merged

ci: add label-gated proxy-test dispatch to databricks-driver-test#799
vikrantpuppala merged 1 commit into
mainfrom
feat/python-proxy-test-dispatch

Conversation

@vikrantpuppala

@vikrantpuppalavikrantpuppala commented May 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Wires this repo into the proxy-based integration test infrastructure in databricks/databricks-driver-test. Mirrors the pattern used by databricks-odbc#55 and the ADBC drivers (C# / Rust).

The driver-test repo already runs proxy-based integration tests against the canonical C# implementation on every PR to it. With this change, the same harness can be triggered against this repo's PRs:

  1. A maintainer reviews a PR and adds the integration-test label.
  2. This workflow dispatches a python-pr-test event to databricks/databricks-driver-test with the PR number + head SHA.
  3. The driver-test repo installs the connector at the dispatched SHA via pip install git+https://...@<sha>, runs the Python proxy test suite (databricks-driver-test#329, tests/python/, ~59 tests in Phase 1) against the pecotesting warehouse with mitmproxy in the loop, and posts a check run back to this PR.

Security

ConcernMitigation
Untrusted PR code running with workspace credentialsLabel-gated: only maintainers can apply integration-test.
Re-running after a malicious follow-up commitAuto-removed on synchronize: pushing new commits clears the label, forcing maintainer re-review before tests re-run. A comment is posted explaining what happened.
Bypassing the gate via the merge queuemerge_group is trusted at that point — dispatches automatically.

The dispatch token comes from a GitHub App (INTEGRATION_TEST_APP_ID / INTEGRATION_TEST_PRIVATE_KEY), not a personal PAT, so it can be scoped to just databricks-driver-test and rotated independently.

Companion PRs

Test plan

  • After merge: create the integration-test label in this repo (one-time manual step — same as we did for databricks-odbc).
  • Add the label to an open PR and verify the python-pr-test dispatch appears in the driver-test Actions tab.
  • Verify the check run shows up on this PR (in-progress → success / failure).
  • Push a new commit to the test PR and verify the label is auto-removed with a comment explaining why.

Out of scope

  • The companion driver-test PR (#329) is responsible for the actual test suite. This PR only adds the dispatch trigger.
  • Phase 1 ports 2 of 17 C# spec files (~59 of ~174 tests). Subsequent phases land in follow-up PRs on the driver-test repo without changes here.

This pull request and its description were AI-assisted by Isaac.

msrathore-db
msrathore-db approved these changes May 22, 2026
View reviewed changes

@msrathore-dbmsrathore-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.

LGTM. Just sanitise the title

msrathore-db
msrathore-db reviewed May 22, 2026
View reviewed changes
Comment thread.github/workflows/trigger-integration-tests.yml Outdated
Comment thread.github/workflows/trigger-integration-tests.yml
@vikrantpuppala
ci: add cross-repo IT dispatch + merge-queue gate
86c8d27
Wires this repo into the proxy-based integration test infrastructure
in databricks/databricks-driver-test. Mirrors the canonical pattern
established in adbc-drivers/databricks (5-job workflow, not the
simpler ODBC variant).
How it works:
- On a normal PR event (open / push / reopen / non-IT label) we
post a `success` Python Proxy Tests check immediately so the
required check doesn't block the PR. The real tests run in the
merge queue.
- When a maintainer adds the `integration-test` label we dispatch
the suite as a preview — useful for catching regressions before
merge queue time. Includes path-filter detection: if only
docs / tests/unit / etc. changed, auto-pass instead of dispatching.
- Pushing new commits auto-removes the label so a subsequent
labelled run requires a fresh maintainer review.
- On the `merge_group` event the suite runs as the real required
gate. Only PRs whose tests dispatch (or auto-pass when no driver
files changed) can proceed to `main`.
Security:
- Label-gated PR preview: only maintainers can apply
`integration-test`. Fork PRs are not auto-trusted.
- Auto-removed on synchronize: pushing new commits clears the
label, forcing maintainer re-review before tests re-run.
- PR titles are sanitized before interpolation into the dispatch
JSON payload (prevents quote/newline-based JSON injection).
- Dispatch failures post a `failure` check-run via a separate
public-repo App token so the gate never silently turns green
when the dispatcher itself broke.
Operational follow-ups (outside this PR):
1. Create the `integration-test` label in this repo (already done).
2. Install `INTEGRATION_TEST_APP_ID` / `INTEGRATION_TEST_PRIVATE_KEY`
secrets for the GitHub App with write access to
databricks/databricks-driver-test.
3. Enable merge queue on `main` branch protection AND mark
`Python Proxy Tests` a required status check. Until this is done
the merge-queue job is dead code and ITs run only on explicit
label.
Addresses review comments on databricks-sql-python#799 (JSON
injection + sparse merge-queue payload) and adopts the
adbc-drivers/databricks canonical pattern wholesale.
Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
vikrantpuppalaforce-pushed the feat/python-proxy-test-dispatch branch from 6d7f15f to 86c8d27CompareMay 22, 2026 09:44
@vikrantpuppala

vikrantpuppala commented May 22, 2026

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review @msrathore-db — both comments uncovered a deeper issue.

Investigating your point about the merge-queue payload prompted me to look at the canonical pattern in adbc-drivers/databricks (the established gold standard for cross-repo IT dispatch in this org). It turns out the original version of this PR — which I'd copied from databricks-odbc#55 — was significantly simpler than the canonical pattern, and the merge-queue traceability gap was a symptom of a bigger missing piece: the merge-queue job is supposed to be the real gate, not a nice-to-have.

I've rewritten the workflow from scratch to match the canonical 4-job pattern (the ADBC version has 5 — extra one is for a second driver in the same repo).

What's new (vs the original 2-job version)

  • skip-integration-tests-pr — On any PR event that isn't a labeled event, post a synthetic successPython Proxy Tests check. Without this, making Python Proxy Tests a required check would block every PR until someone applied the label.
  • merge-queue-python — Real gate on merge_group. Detects whether driver source files (or workflow / pyproject / lockfile) actually changed; if not, auto-pass; if yes, dispatch with the full traceability payload extracted from the queue ref. Posts a failure check if dispatch itself errors.
  • trigger-tests-pr — Same dispatch path as before, but now also has changed-paths detection (auto-pass when no driver files touched), title sanitization (your F1), and a dispatch-failure handler.

Both your comments addressed

  • F1 — JSON injection in pr_title: sanitized via actions/github-script with result-encoding: string, applied in both PR-label and merge-queue dispatch paths.
  • F2 — Sparse merge-queue payload: PR number is now extracted from the merge-queue head ref (pr-<N>-<sha> pattern); full payload (pr_url, placeholder pr_title/pr_author) matches the labeled-PR shape so the driver-test repo's check-run titles don't have to special-case merge-queue runs.

Operational follow-ups (outside this PR)

For this to deliver value, after merge someone needs to:

  1. Enable merge queue on main branch protection.
  2. Mark Python Proxy Tests as a required status check.
  3. Install INTEGRATION_TEST_APP_ID / INTEGRATION_TEST_PRIVATE_KEY secrets (the GitHub App with write access to databricks-driver-test).

The integration-test label has already been created.

Until step 1+2 are done, the merge-queue job is dead code and ITs run only on explicit label — same effective behaviour as the original version. The label path is fully usable in that interim state.

Re-pushed via --force-with-lease. PR should be ready for another look.

@vikrantpuppala
vikrantpuppala merged commit fb55001 into mainMay 22, 2026
39 checks passed
This was referenced May 22, 2026
Closed
Merged
vikrantpuppala added a commit that referenced this pull request May 25, 2026
@vikrantpuppala
ci(integration-tests): use github.token for check-run posters (#801)
60060c9
* ci(integration-tests): use github.token for check-run posters
Follow-up to #799. The dispatch failure handlers and auto-pass
steps were posting check-runs with `steps.public-token.outputs.token`,
which is itself an App-token-generating step.
That created a silent-failure trap: if the App secrets are
missing or rotated, the App-token step fails, then the failure
handler also fails (no token to authenticate with), and the gate
sits green from the earlier `skip-integration-tests-pr` job's
synthetic-success check — the exact silent-pass anti-pattern
the failure handler exists to prevent.
Discovered by exercising the dispatch end-to-end on a draft PR
before the App secrets were installed (#800 closed). The
canonical adbc-drivers/databricks workflow has the same latent
bug — fix not yet upstreamed there.
The fix is to use the default workflow `${{ github.token }}` for
all check-posting steps. The default token already has
`checks: write` because each job declares the permission.
`steps.public-token` is no longer referenced anywhere; the
generation step is removed to keep the workflow tidy.
The App token is still used (correctly) for the actual dispatch
call into databricks-driver-test, where cross-repo write access
is required.
Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
* test(retry): isolate retry tests from shared telemetry executor
The e2e retry tests in retry_test_mixins.py assert exact urllib3 call
counts (e.g. `mock_validate_conn.call_count == 6`) while mocking
urllib3 globally via `_validate_conn` / `_get_conn`. The shared
`TelemetryClientFactory` executor — populated by *prior* tests in the
same worker — keeps firing background telemetry pushes that land on the
same mocked urllib3 layer, inflating the counts and tripping the
assertions.
Drain the factory and route any new `initialize_telemetry_client` call
to `NoopTelemetryClient` for the duration of the three tests that
assert exact counts:
- test_oserror_retries
- test_retry_max_count_not_exceeded
- test_retry_exponential_backoff
Factory state is fully restored on exit so subsequent tests see the
real telemetry pipeline.
Co-authored-by: Isaac
---------
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msrathore-dbmsrathore-dbmsrathore-db approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vikrantpuppala@msrathore-db