Fixes #28981: Migrate salesforce, domodatabase, saperp, sas, deltalake, burstiq to BaseConnection#28982
Merged
Merged
Conversation
…BaseConnection The final non-Engine connectors. Each returns a native client (simple-salesforce Salesforce, pydomo Domo, SapErpClient, SASClient, DeltalakeClient, BurstIQClient); build and test move into the connection class. deltalake keeps get_deltalake_client + DeltalakeClient module-level; burstiq keeps get_connection (+ its client cache) module-level since metadata.py imports it, with _get_client delegating. Logic preserved verbatim; specs wire connection_class. burstiq's connection test repointed to the new class method.
github-actions
Bot
added
Ingestion
safe to test
…et_client Moving get_connection into _get_client shifted three pre-existing baselined reportArgumentType errors (password/security_token/consumer_secret accept CustomSecretStr|str|None where simple-salesforce expects str|None). Invisible locally without the simple-salesforce SDK; carry inline ignores.
Contributor
🟡 Playwright Results — all passed (8 flaky)✅ 4306 passed · ❌ 0 failed · 🟡 8 flaky · ⏭️ 88 skipped
🟡 8 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
… SDK test_domodatabase/test_salesforce patched pydomo.Domo / simple_salesforce.api.Salesforce, but the connection modules do 'from pydomo import Domo' / 'from simple_salesforce.api import Salesforce', so the binding the code uses is <connector>.connection.Domo/Salesforce. The SDK-level patch only worked when the connection module happened to be first imported during the test (so the from-import grabbed the mock). Once any other test imports the connection module first (e.g. the new colocated test), the patch became inert and the real client authenticated -> invalid_client/ExpatError. Patch where the name is looked up.
|
pmbrull
approved these changes
Jun 12, 2026
Code Review ✅ ApprovedCompletes the migration of all remaining database connectors to the BaseConnection architecture. The implementation maintains existing functionality for the six specified connectors with no issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
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.
Fixes #28981
What
The final batch — migrates the last six non-
Engineconnectors: salesforce, domodatabase, saperp, sas, deltalake, burstiq. With this, every database connector is onBaseConnection.Shape
Each returns a native client (
C= that client), andtest_connectiondrives native calls viaself.client:CSalesforceDomoSapErpClientSASClientDeltalakeClient(PySpark/S3)BurstIQClient(cached)Kept module-level (imported elsewhere)
get_deltalake_client(singledispatch) +DeltalakeClient;_get_clientbuilds the client through them.get_connection+ its SHA-256 client cache (metadata.pyimportsget_connection);_get_clientdelegates.Tests
Colocated unit tests (subclassing, guarded with
importorskipfor optional SDKs); existing topology tests pass (domodatabase, saperp, sas, burstiq).burstiq's connection test repointed to the new class method. ruff + basedpyright clean (pre-existing baselined optional-access carried as inline ignores; salesforce/simple-salesforce import resolves in CI). Logic preserved verbatim.A live e2e isn't run here (each needs a real Salesforce/Domo/SAP/SAS/Delta/BurstIQ backend).