Migrate Teradata test connection to the @check framework (#31642) - #33140
Merged
Conversation
* feat(ingestion): migrate Teradata test connection to the @check framework Teradata was still on the legacy test_connection_db_common path, so every failure surfaced as the raw teradatasql exception with no structured diagnosis - a bad password rendered as a 25-line Go stack trace from gosqldriver/teradatasql. - TeradataChecks implements the seeded steps on the shared ping/list_*/ run_sql helpers, and inherits the TCP reachability preflight, so an unreachable host fails as a network problem before credentials are exercised. - TERADATA_ERRORS maps the driver's own codes to diagnoses. teradatasql exposes no .errno/.sqlstate, so both are read back out of the message text, which is where the driver puts them - but the codes are stable and locale-independent unlike the prose that follows. Only codes confirmed against Teradata's published references are encoded; anything else keeps its raw errorLog rather than getting a guessed diagnosis. Auth is keyed on SQLState 28000 rather than Error 8017 alone so the LDAP and Kerberos logmech rejections, which carry their own codes, are covered too. - teradata.json: CheckAccess gains category=ConnectionGate. The runner short-circuits on the category, not the legacy shortCircuit flag, so without it a failed gate let every later step open its own connection. A test asserts this and fails when the line is removed. - teradata.json: add the GetDatabases step. The legacy handler already passed TERADATA_GET_DATABASE as a GetDatabases query, but the seeded definition had no such step and the runner iterates the definition - so that query had never run. - _get_client registers engine.dispose for deterministic teardown. Tests cover each classifier rule, rule precedence where two could match, that a bare number in a message is not read as a code, that the provider's checks match the seeded definition exactly, and an end-to-end runner pass asserting the gate diagnosis and that later steps skip. * fix(ingestion): make the new Teradata GetDatabases step non-mandatory The step is newly introduced - the legacy handler passed the query but the seeded definition had no step for it, so it never ran. As mandatory it would newly fail a service whose user cannot read dbc.databasesvx but which tests and ingests fine today, and a failed mandatory step also aborts ingestion. Non-mandatory keeps the diagnosis without the regression. Also: - Reword the auth rule's rationale to what is sourced. Error 8017 under SQLState 28000 is what Teradata reports for both LOGMECH=TD2 and LOGMECH=LDAP; the earlier comment claimed the class additionally covered Kerberos rejections carrying their own codes, which was not verified. The rule is unchanged - keying on the standard class rather than a single code stands on its own, without asserting coverage that has not been observed. - Cover get_databases' success path, which no test reached: the runner tests all fail at the gate, so the summarizer had no runtime coverage at all. Three cases - a normal count, an empty result, and the sample cap. * fix(ingestion): diagnose Teradata network failures the Go driver reports itself A hostname typo came back as a raw 25-line stack trace with no diagnosis. Two separate causes: The reachability preflight never ran. ping() derives the probe target from the engine URL and skips the probe when no port is present - and Teradata's hostPort is commonly a bare hostname, so the skip was the normal case, not the edge case. check_access now probes the port the driver would dial, defaulting to 1025, so a DNS or firewall failure is caught in Python and fails fast. Nothing in the error pack could catch what got through. teradatasql is a Go driver behind cgo: it resolves the hostname itself and reports the failure as its own OperationalError, with no Python socket exception in the chain. NETWORK_ERRORS matches by exception type, so it is structurally blind to every network failure teradatasql detects on its own. Added Error 493 (hostname lookup, observed) and a generic rule on SQLState 08000, the SQL-standard connection-exception class, as the backstop for whatever still reaches the driver. Tests cover both codes, the rule ordering between them, and that the preflight probes the configured port when given and 1025 when not. * refactor(ingestion): match Teradata codes with the shared Matchers.contains Review feedback. The bespoke regex matchers were compared against Matchers.contains across every case the tests cover - both codes, both SQLStates, the rule-ordering cases and the bare-number false positive - and agreed on all 28 combinations. The brackets alone already prevent a number elsewhere in the message being read as a code, which was the only thing the regex was there to guarantee. So the custom machinery bought nothing: two compiled patterns, three helpers and a Matcher import deleted in favour of the shared matcher. The comment explaining why the rules key on the bracketed code rather than the prose after it is kept - that reasoning is what makes the rules stable across locales, and it survives the implementation change. All 26 tests pass untouched, which is itself evidence the two forms are equivalent. Also use the real driver exception in the tests. They previously subclassed sqlite3.OperationalError and justified it by saying teradatasqlalchemy is absent from the unit test environment - that was wrong, generalised from a local venv rather than checked. setup.py's test_unit includes plugins[teradata], so CI has the dialect. The tests now subclass teradatasql.OperationalError and build a real teradatasql engine, so SQLAlchemy wraps the error exactly as it does in production. sqlite remains only where a genuinely queryable engine is needed, and says so. * test(ingestion): keep the Teradata test module importable without the driver gitar-bot review. The previous commit made this module hard-import teradatasql at collection time, so every test in it - including the ~23 that only call ErrorPack.classify on a synthetic exception - would fail to collect wherever the optional teradata extra is not installed. A collection error is the worst version of this: it takes the whole file down rather than skipping the part that needs the driver. The driver was never needed. run_sql wraps whatever connect raises into a CheckError and the error pack reads the message text, so the exception's type and DBAPI lineage are immaterial to every assertion here. Verified by uninstalling teradatasql and teradatasqlalchemy entirely: all 26 pass. So the stand-in is a plain Exception carrying a real driver message, and the engine is sqlite because the dialect is incidental to what is asserted. Both now say that, rather than the previous two justifications - one of which claimed CI lacks the extra, which was wrong. --------- Co-authored-by: Khairajani <himanshu.k@deuexsolutions.com> (cherry picked from commit 8ad603b)
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Contributor
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ ApprovedMigrates Teradata test connection to the OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Contributor
✅ Playwright Results — workflow succeededValidated commit ✅ 620 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 2m 28s ⏱️ Max setup 3m 55s · max shard execution 13m 12s · max shard-job elapsed before upload 18m 50s · reporting 5s 🌐 214.40 requests/attempt · 2.74 app boots/UI scenario · 6.69% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Khairajani
added
safe to test
Khairajani
enabled auto-merge (squash)
September 10, 2026 13:54
mohittilala
approved these changes
Sep 10, 2026
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.
Backport of #31642 to 2.0.
git cherry-pick -xwith one conflict inconnection.py, resolved by taking the incoming side: 2.0's only drift in that file wasOptional[...]styling inside thetest_connectionmethod the migration removes. All three files are byte-identical tomain.🤖 Generated with Claude Code