fix(sigma): populate dataModels field on dashboard during ingestion - #29608
fix(sigma): populate dataModels field on dashboard during ingestion#29608zak-nuccio wants to merge 3 commits into
Conversation
❌ 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! |
![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
This PR fixes Sigma dashboard ingestion to populate the dataModels field on CreateDashboardRequest, aligning Sigma behavior with other dashboard connectors that link dashboards to their data models via FQNs.
Changes:
- Populate
CreateDashboardRequest.dataModelsinSigmaSource.yield_dashboard()whenincludeDataModelsis enabled (otherwise set it toNone). - Update the Sigma unit test expected dashboard to include
dataModels. - Add unit tests to validate
dataModelspopulation when enabled andNonewhen disabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ingestion/src/metadata/ingestion/source/dashboard/sigma/metadata.py | Adds dataModels FQN list to the dashboard create request, gated by includeDataModels. |
| ingestion/tests/unit/topology/dashboard/test_sigma.py | Updates expected request + adds tests for enabled/disabled dataModels behavior. |
| self.sigma.source_config.includeDataModels = True | ||
| self.sigma.context.get().__dict__["dataModels"] = ["elem1", "elem2"] | ||
|
|
||
| results = list(self.sigma.yield_dashboard(MOCK_DASHBOARD_DETAILS)) | ||
| dashboard_request = results[0].right | ||
|
|
||
| self.assertIsNotNone(dashboard_request.dataModels) | ||
| self.assertEqual(len(dashboard_request.dataModels), 2) | ||
| for dm_fqn in dashboard_request.dataModels: | ||
| self.assertIn("mock_sigma", dm_fqn.root) | ||
|
|
deb5bd3 to
f5bbc8c
Compare
|
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! |
|
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! |
| def test_yield_dashboard_with_datamodels(self): | ||
| """ | ||
| Function for testing that dashboard includes dataModels when includeDataModels is True | ||
| """ | ||
| self.sigma.source_config.includeDataModels = True | ||
| original_data_models = self.sigma.context.get().dataModels | ||
| self.sigma.context.get().__dict__["dataModels"] = ["elem1", "elem2"] | ||
|
|
||
| try: | ||
| results = list(self.sigma.yield_dashboard(MOCK_DASHBOARD_DETAILS)) | ||
| dashboard_request = results[0].right | ||
|
|
||
| self.assertIsNotNone(dashboard_request.dataModels) | ||
| self.assertEqual(len(dashboard_request.dataModels), 2) | ||
| self.assertEqual(dashboard_request.dataModels[0].root, "mock_sigma.elem1") | ||
| self.assertEqual(dashboard_request.dataModels[1].root, "mock_sigma.elem2") | ||
| finally: | ||
| self.sigma.context.get().__dict__["dataModels"] = original_data_models | ||
|
|
| def test_yield_dashboard_datamodels_disabled(self): | ||
| """ | ||
| Function for testing that dataModels is None when includeDataModels is False | ||
| """ | ||
| self.sigma.source_config.includeDataModels = False | ||
|
|
||
| results = list(self.sigma.yield_dashboard(MOCK_DASHBOARD_DETAILS)) | ||
| dashboard_request = results[0].right | ||
|
|
||
| self.assertIsNone(dashboard_request.dataModels) | ||
|
|
|
Anything else I need to do to move this over the line? |
|
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! |
|
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! |
| self.assertEqual(dashboard_request.dataModels[0].root, "mock_sigma.elem1") | ||
| self.assertEqual(dashboard_request.dataModels[1].root, "mock_sigma.elem2") |
|
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! |
|
Hey @zak-nuccio, can you please address the PR comments? couple ones from the bots are relevant |
|
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! |
SigmaSource.yield_dashboard() was creating CreateDashboardRequest without the dataModels field. The topology processes yield_datamodel before yield_dashboard, so self.context.get().dataModels already holds the element names — this change passes them through to the API as FullyQualifiedEntityName references, matching the pattern used by the Tableau connector. Tests: - Updated EXPECTED_DASHBOARD to include dataModels=[] - test_yield_dashboard_with_datamodels: verifies 2 data model FQNs appear when includeDataModels=True - test_yield_dashboard_datamodels_disabled: verifies dataModels is None when includeDataModels=False
Copilot flagged state leak — test_yield_dashboard_with_datamodels and test_yield_dashboard_datamodels_disabled mutated source_config.includeDataModels without restoring the original value, making later tests order-dependent. Save and restore the original flag in a finally block on both tests.
…odels test The fqn.build for DashboardDataModel produces '<service>.model.<name>' (confirmed at fqn.py:586), so the test assertion needs the 'model.' segment. Also convert to plain pytest-style assert per review feedback.
b27b846 to
3daab96
Compare
|
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 ✅ Approved 1 resolved / 1 findingsPopulates the dataModels field on Sigma dashboards during ingestion by mirroring the Tableau connector pattern, with new unit tests validating both enabled and disabled states. No issues found. ✅ 1 resolved✅ Quality: New tests use unittest asserts instead of plain assert
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
🔴 Playwright Results — 16 pipeline/setup failure(s)✅ 0 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped Pipeline and setup failures
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
@IceS2 rebased on latest main and addressed the review feedback:
Mind taking another look? |
| dataModels=[ | ||
| FullyQualifiedEntityName( | ||
| fqn.build( | ||
| self.metadata, | ||
| entity_type=DashboardDataModel, | ||
| service_name=self.context.get().dashboard_service, | ||
| data_model_name=data_model, | ||
| ) | ||
| ) | ||
| for data_model in self.context.get().dataModels or [] | ||
| ] | ||
| if self.source_config.includeDataModels | ||
| else None, |
Fixes #29607
Applied the same comprehension guarded by
self.source_config.includeDataModelsthat the Tableau connector uses.EXPECTED_DASHBOARDin the Sigma test to include the new fieldtest_yield_dashboard_with_datamodels(2 data models in context, asserts both FQNs on the dashboard)test_yield_dashboard_datamodels_disabled(assertsdataModelsisNonewhen toggled off)Greptile Summary
This PR fixes the Sigma connector so that data models ingested via
yield_datamodelare properly referenced in theCreateDashboardRequestduringyield_dashboard. Previously, thedataModelsfield was never populated, so dashboards had no link to their data models.dataModelslist comprehension inyield_dashboardthat builds FQNs fromself.context.get().dataModels, guarded byself.source_config.includeDataModels(returnsNonewhen disabled).EXPECTED_DASHBOARDin the Sigma unit test to includedataModels=[]and adds two targeted tests covering the enabled and disabled flag paths.Confidence Score: 5/5
Safe to merge — the change is a narrow, well-tested addition that populates a previously empty field.
The fix mirrors an established pattern already used by the Tableau connector. The
or []guard prevents iteration errors when no data models are in context, theelse Nonebranch correctly signals disabled state, and three test cases cover the enabled path (with FQN assertions), the disabled path, and the pre-existing baseline. No existing behavior is altered.No files require special attention.
Important Files Changed
dataModelscomprehension toyield_dashboard, correctly guarded byincludeDataModels; mirrors the Tableau connector pattern.EXPECTED_DASHBOARDand adds two new tests coveringincludeDataModels=True(with FQN assertions) andincludeDataModels=False(expectsNone).Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant T as Topology Engine participant DM as yield_datamodel participant CTX as Context (dataModels[]) participant YD as yield_dashboard T->>DM: call yield_datamodel(details) DM->>DM: "filter by includeDataModels & vizualizationType" DM->>CTX: register_record_datamodel appends elementId T->>YD: call yield_dashboard(details) YD->>CTX: read dataModels list alt "includeDataModels = True" YD->>YD: build FQNs for each dataModel YD-->>T: CreateDashboardRequest with dataModels list else "includeDataModels = False" YD-->>T: "CreateDashboardRequest with dataModels=None" end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant T as Topology Engine participant DM as yield_datamodel participant CTX as Context (dataModels[]) participant YD as yield_dashboard T->>DM: call yield_datamodel(details) DM->>DM: "filter by includeDataModels & vizualizationType" DM->>CTX: register_record_datamodel appends elementId T->>YD: call yield_dashboard(details) YD->>CTX: read dataModels list alt "includeDataModels = True" YD->>YD: build FQNs for each dataModel YD-->>T: CreateDashboardRequest with dataModels list else "includeDataModels = False" YD-->>T: "CreateDashboardRequest with dataModels=None" endReviews (8): Last reviewed commit: "fix(sigma): use pytest-style asserts and..." | Re-trigger Greptile