-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ingest): Generate browse paths v2 for more sources; properly pass platform_instance #8501
fix(ingest): Generate browse paths v2 for more sources; properly pass platform_instance #8501
Conversation
… platform_instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I scanned thru all the goldens and changes looked reasonable to me. One question around the "remote" source -- which is the csv_enricher / datahub-lineage-file / datahub-business-glossary. Not sure whether we should be auto generating stuff for them or not
@@ -139,8 +143,11 @@ def load_lineage_config(file_name: str) -> LineageConfig: | |||
lineage_config = LineageConfig.parse_obj(config) | |||
return lineage_config | |||
|
|||
def get_workunits(self) -> Iterable[MetadataWorkUnit]: | |||
return auto_workunit_reporter(self.report, self.get_workunits_internal()) | |||
def get_workunit_processors(self) -> List[Optional[MetadataWorkUnitProcessor]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually defined here since this is a "file" ingestion source and want to keep the output more constrained. I don't really know how this is used though, so if you think it's better to get all the helpers I'll remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this makes sense
"entityType": "tag", | ||
"entityUrn": "urn:li:tag:Legacy", | ||
"changeType": "UPSERT", | ||
"aspectName": "tagKey", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting a tag key aspect, just want to make sure this makes sense for this source
@@ -2,10 +2,6 @@ | |||
from dataclasses import dataclass, field | |||
from typing import Dict, Iterable, List, Optional, Tuple, Type, Union, ValuesView | |||
|
|||
import bson | |||
import bson.dbref | |||
import bson.int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these just unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I just ran the intellij "optimize imports" command on a bunch of files because they had imports to delete. It's very possible these are needed, we'll see what CI says I guess
Merged as ingestion tests are passing. |
Makes the following changes:
platform_instance
properly as per fix(ingest/browsePathv2): fix passing platform instance correctly #8478, thanks @anshbansal!get_workunits
in many sources so that they run workunit processors, which will make them both (i) create status aspects and (ii) create browse paths v2Checklist