Migrate Teradata test connection to the @check framework - #31642
Conversation
…work 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.
❌ 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 |
|
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! |
✅ Playwright Results — workflow succeededValidated commit ✅ 4485 passed · ❌ 0 failed · 🟡 9 flaky · ⏭️ 1 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 0m 18s ⏱️ Max setup 6m 9s · max shard execution 22m 32s · max shard-job elapsed before upload 25m 36s · reporting 22s 🌐 216.89 requests/attempt · 2.31 app boots/UI scenario · 31.36% common-shard skew Optimization targets still in progress:
🟡 9 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 |
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.
|
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! |
Khairajani
added
safe to test
…rts 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.
🚦 Removed from the merge queue —
|
Code Review ✅ Approved 1 resolved / 1 findingsMigrates the Teradata test connection framework to use structured checks and error diagnoses. No issues found. ✅ 1 resolved✅ Quality: Test module now hard-imports teradatasql at collection time
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 |
|
3 participants
Describe your changes:
Fixes #
Teradata was still on the legacy
test_connection_db_commonpath, so every test connection failure surfaced as the rawteradatasqlexception with no structured diagnosis. A wrong password rendered as a 25-linegosqldriver/teradatasqlGo stack trace.This migrates it onto the
@check/ChecksProviderframework, following the same shape as the Postgres (#29342) and MySQL (#28793) migrations.Changes
TeradataChecksimplements the seeded steps on the sharedping/list_schemas/list_tables/list_views/run_sqlhelpers — no new abstraction. It inherits the shared TCP reachability preflight, so an unreachable host now fails as a network problem before credentials are exercised.TERADATA_ERRORSmaps the driver's own codes to diagnoses.teradatasqlexposes no.errno/.sqlstateattribute — it raises a singleOperationalErrorwhose message embeds both, e.g.so both codes are read back out of the text. The codes themselves are stable and locale-independent, unlike the prose that follows them.
Auth is keyed on SQLState 28000 (the SQL-standard "invalid authorization specification" class) rather than
Error 8017alone. 8017-under-28000 is what Teradata reports forLOGMECH=TD2andLOGMECH=LDAPalike; keying on the class means any other rejection Teradata files as an authorization failure is covered without the pack enumerating — or guessing at — codes that have not been observed.NETWORK_ERRORSis composed in via.including.Only codes whose text is confirmed against Teradata's published references are encoded (8017/28000, 3802, 3807, 3523). Anything else keeps its raw
errorLograther than being given a guessed diagnosis.teradata.json:CheckAccessgainscategory: "ConnectionGate". The runner short-circuits on the category, not the legacyshortCircuitflag. Without it a failed gate lets every later step open its own connection — the multi-minute hang measured in Fixes #29341: migrate Postgres test connection to the @check framework #29342. A test asserts this and fails when the line is removed.teradata.json: adds theGetDatabasesstep,mandatory: false. Not scope creep: the legacy handler already passedTERADATA_GET_DATABASEas aGetDatabasesquery, but the seeded definition had no such step andtest_connection_stepsiterates the definition — so that query had never actually run. It is deliberately not mandatory: the step is new, so making it required would newly fail a service whose user cannot readdbc.databasesvxbut which tests and ingests fine today — and a failed mandatory step also aborts ingestion viaraise_test_connection_exception. A test pins this._get_clientregistersengine.disposefor deterministic teardown.Related: #31639 fixes the UI side — the failure card was rendering
errorLogin preference to the step'smessage, which is why the Teradata trace filled the modal. That fix helps every connector without an error pack; this PR is what makes Teradata's failures sharp rather than merely readable.Type of change:
High-level design:
N/A — follows the established per-connector migration pattern (#29342, #28793); no new shared abstractions.
Tests:
Use cases covered
LOGMECH=LDAP, which reports the same 8017/28000 pairing; and any other rejection Teradata files under SQLState 28000 classifies without the pack having to know its code.Skipped/ConnectionNotEstablishedrather than each opening its own connection.errorLog, with no diagnosis invented for it.GetDatabasesreports what it found, including an empty result and the sampling cap, without failing the connection test.Unit tests
Added.
File:
ingestion/tests/unit/source/database/teradata/test_connection.py— 21 tests, all passing."query returned 3802 rows") is not read as a code.test_checks_cover_exactly_the_seeded_steps— the provider's checks match the seededteradata.json, read from the resource rather than transcribed, so the two cannot drift.test_the_seeded_gate_step_is_tagged_as_the_connection_gate— verified load-bearing: removing thecategoryline makes this and the short-circuit test fail (later steps goFailedinstead ofSkipped).TestConnectionRunnerpass over the real seeded definition, asserting the gate diagnosis and the skip cascade.GetDatabasessuccess path — normal count, empty result, and theDEFAULT_SAMPLE_ROWScap. Every runner test fails at the gate, so without these the summarizer had no runtime coverage at all.test_the_new_get_databases_step_is_not_mandatory— pins the decision above so it cannot be flipped without a reviewer seeing it.Note: the tests deliberately do not require the
teradatasqlalchemydialect (an optional extra, absent from the base unit-test environment). The driver-error stub subclassessqlite3.OperationalErrorso a plain sqlite engine wraps it into aDBAPIErrorin exactly the production shape; the classifier reads the message, which is identical either way.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
The originating report came from a live Teradata 20.0 system: an invalid credential test connection returned
[Error 8017] [SQLState 28000] The UserId, Password or Account is invalid.— that exact message is reproduced verbatim in the unit tests. Re-verification against the live system is pending.Checks run
(Collection errors in a wider local run are missing optional driver extras —
pyathena,pyodbc,teradatasqlalchemy— not related to this change.)Seeding was checked:
TestConnectionDefinitionResourcecallsrepository.createOrUpdate(...)on startup, so upgraded deployments do receive the newcategoryand the added step rather than keeping a stale row.UI screen recording / screenshots:
Not applicable — no UI changes.
Checklist:
I have read the CONTRIBUTING document.
My PR title is
Fixes <issue-number>: <short explanation>My PR is linked to a GitHub issue via
Fixes #<issue-number>above.I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: no schema change — only a seeded data resource.
I have added tests and listed them above.
I have added tests around the new logic.
For connector/ingestion changes: no user-facing documentation change needed — the step list and diagnoses are self-describing in the UI.
Greptile Summary
The PR migrates Teradata connection testing from the legacy database helper to the structured checks framework.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Runner as TestConnectionRunner participant Checks as TeradataChecks participant Network as TCP preflight participant DB as Teradata Runner->>Checks: CheckAccess Checks->>Network: Probe configured host and port alt Network unavailable Network-->>Runner: Classified network failure Runner-->>Runner: Skip remaining steps else Network reachable Checks->>DB: SELECT 1 alt Authentication or connection failure DB-->>Runner: Classified Teradata error Runner-->>Runner: Skip remaining steps else Connection established Runner->>Checks: GetDatabases Runner->>Checks: GetSchemas Runner->>Checks: GetTables Runner->>Checks: GetViews end endReviews (7): Last reviewed commit: "test(ingestion): keep the Teradata test ..." | Re-trigger Greptile
Context used: