Conversation
github-actions
bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds schema and backend secrets-conversion support for connecting to Matillion Data Productivity Cloud (DPC) alongside the existing Matillion ETL connection type.
Changes:
- Extend
MatillionConnectionto allow a newMatillionDPCauth config and introducelineageLookbackDays. - Add a new JSON schema defining the Matillion DPC authentication configuration (OAuth2 client credentials or PAT).
- Update the backend
MatillionConnectionClassConverterto convert/mask/unmask secrets for the new DPC auth type.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openmetadata-spec/src/main/resources/json/schema/entity/services/connections/pipeline/matillionConnection.json | Adds MatillionDPC as a supported connection option and introduces lineageLookbackDays for DPC OpenLineage lookback. |
| openmetadata-spec/src/main/resources/json/schema/entity/services/connections/pipeline/matillion/matillionDPC.json | New schema defining Matillion DPC auth fields (client credentials, PAT, region). |
| openmetadata-service/src/main/java/org/openmetadata/service/secrets/converter/MatillionConnectionClassConverter.java | Ensures secrets conversion supports both MatillionETLAuth and the new MatillionDPCAuth. |
| "format": "password" | ||
| } | ||
| }, | ||
| "required": [], |
There was a problem hiding this comment.
MatillionDPC auth config currently has an empty required list, which means an empty object (or one with only region) can pass schema validation. This will allow invalid connections to be saved and then fail later at runtime. Consider enforcing that either personalAccessToken is provided, or both clientId and clientSecret are provided (and ideally disallow providing both auth methods at once).
| "required": [], | |
| "required": [], | |
| "anyOf": [ | |
| { | |
| "required": [ | |
| "personalAccessToken" | |
| ] | |
| }, | |
| { | |
| "required": [ | |
| "clientId", | |
| "clientSecret" | |
| ] | |
| } | |
| ], |
| @@ -36,10 +39,18 @@ | |||
| "$ref": "../../../../type/filterPattern.json#/definitions/filterPattern", | |||
| "title": "Default Pipeline Filter Pattern" | |||
| }, | |||
| "lineageLookbackDays": { | |||
| "title": "Lineage Lookback Days", | |||
| "description": "Number of days to look back when fetching lineage events from Matillion DPC OpenLineage API.", | |||
| "type": "integer", | |||
| "default": 30, | |||
| "minimum": 1, | |||
| "maximum": 365 | |||
| }, | |||
There was a problem hiding this comment.
New schema fields (MatillionDPC auth option and lineageLookbackDays) aren’t covered by the existing configuration parsing tests for Matillion. Adding a unit test that parses a valid DPC config (PAT and/or client credentials) and asserts validation failures for missing credentials / out-of-range lineageLookbackDays would prevent regressions in schema-to-model generation and validation behavior.
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
🟡 Playwright Results — all passed (18 flaky)✅ 3421 passed · ❌ 0 failed · 🟡 18 flaky · ⏭️ 216 skipped
🟡 18 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 |
| matillionConnection.getConnection(), | ||
| List.of(MatillionETLAuth.class, MatillionDPCAuth.class)) |
There was a problem hiding this comment.
The indentation in this multi-line tryToConvertOrFail(...) call doesn’t match the project’s standard Java formatting (and is likely to be rewritten by Spotless / google-java-format). Please reformat this block (e.g., by running mvn spotless:apply) so the continuation indentation is consistent and CI formatting checks don’t fail.
| matillionConnection.getConnection(), | |
| List.of(MatillionETLAuth.class, MatillionDPCAuth.class)) | |
| matillionConnection.getConnection(), | |
| List.of(MatillionETLAuth.class, MatillionDPCAuth.class)) |
OpenMetadata Service New-Code Coverage✅ PASS. Required changed-line coverage:
Only changed executable lines under |
Jest test CoverageUI tests summary
|
| "properties": { | ||
| "type": { | ||
| "title": "Service Type", | ||
| "description": "Service Type", | ||
| "$ref": "#/definitions/matillionType", | ||
| "default": "Matillion" | ||
| }, | ||
| "connection": { | ||
| "title": "Matillion Connection", | ||
| "description": "Matillion Auth Configuration", | ||
| "oneOf": [ | ||
| { | ||
| "$ref": "matillion/matillionETL.json" | ||
| }, | ||
| { | ||
| "$ref": "matillion/matillionDPC.json" | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
MatillionConnection does not declare any required fields (unlike most other pipeline connection schemas). With the new oneOf for connection, this means the top-level config can omit connection entirely and still validate, which is likely not a usable Matillion configuration. Consider adding a required list (at least connection, and possibly type if intended) to prevent silently-accepted invalid configs.
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
|
Failed to cherry-pick changes to the 1.12.4 branch. |
|
|
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
MatillionDPCAuthclass with OAuth2 client credentials and personal access token supportlineageLookbackDaysconfiguration parameter (1-365 days, default 30) for OpenLineage APIMatillionConnectionClassConverterto support bothMatillionETLAuthandMatillionDPCAuthMatillionConnectionClassConverterTestwith ETL/DPC auth conversion testsmatillionDPC.jsonschema with region enum (us1, eu1)matillionConnection.jsonto include DPC auth as oneOf optionThis will update automatically on new commits.