Conversation
github-actions
bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
.../main/resources/json/schema/entity/services/connections/pipeline/matillion/matillionDPC.json
Show resolved
Hide resolved
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 (8 flaky)✅ 2799 passed · ❌ 0 failed · 🟡 8 flaky · ⏭️ 189 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 |
| 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
|
| "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.
💡 Quality: lineageLookbackDays is DPC-specific but on shared connection
The lineageLookbackDays property is placed at the top-level matillionConnection.json schema, meaning it will be visible for both ETL and DPC connection types. However, its description explicitly says it's for the "Matillion DPC OpenLineage API." If ETL connections don't use lineage lookback, this could confuse users configuring an ETL connection. Consider either:
- Moving it inside the
matillionDPC.jsonschema, or - Updating the description to be generic if it applies to both types.
Suggested fix:
Either move lineageLookbackDays into matillionDPC.json, or update the description to not mention DPC specifically if it applies to both connection types.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
.../main/java/org/openmetadata/service/secrets/converter/MatillionConnectionClassConverter.java
Show resolved
Hide resolved
Code Review 👍 Approved with suggestions 1 resolved / 2 findingsAdds Matillion Data Cloud support with ingestion integration. Consider moving the DPC-specific 💡 Quality: lineageLookbackDays is DPC-specific but on shared connectionThe
Suggested fix✅ 1 resolved✅ Edge Case: MatillionDPC schema has no required fields, allowing empty config
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| self.assertIn( | ||
| "We encountered an error parsing the configuration of your MatillionConnection.\nYou might need to review your config based on the original cause of this failure:\n\t - Missing parameter in ('connection', 'hostPort')\n\t - Missing parameter in ('connection', 'username')\n\t - Missing parameter in ('connection', 'password')", | ||
| "We encountered an error parsing the configuration of your MatillionConnection.\n" | ||
| "You might need to review your config based on the original cause of this failure:\n" | ||
| "\t - Missing parameter in ('connection', 'function-after[parse_name(), MatillionEtlAuthConfig]', 'hostPort')\n" | ||
| "\t - Missing parameter in ('connection', 'function-after[parse_name(), MatillionEtlAuthConfig]', 'username')\n" | ||
| "\t - Missing parameter in ('connection', 'function-after[parse_name(), MatillionEtlAuthConfig]', 'password')\n" | ||
| "\t - Invalid parameter value for ('connection', 'function-after[parse_name(), MatillionDpcAuthConfig]', 'type')", |
There was a problem hiding this comment.
This assertion depends on Pydantic's internal error loc formatting (e.g., function-after[parse_name(), ...]) and on the exact ordering of validation errors, which is brittle across Pydantic/model changes. Prefer asserting on smaller, stable substrings (e.g., that the message mentions MatillionConnection and that hostPort/username/password are missing) rather than the full, fully-qualified loc tuples.
|
|
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
MatillionDPCAuthschema with OAuth2 and PAT support inmatillionDPC.jsonMatillionConnectionClassConverterto handle bothMatillionETLAuthandMatillionDPCAuthtypeslineageLookbackDaysparameter (1–365 days, default 30) toMatillionConnectionfor OpenLineage API queriesMatillionConnectionClassConverterTestwith ETL/DPC conversion and null-safety testsClassConverterFactoryTestto includeMatillionConnectionMatillionDPCtype andRegionenumThis will update automatically on new commits.