feat(ingestion): extend feast plugin to ingest tags and owners #11784
Conversation
|
|
||
| for mapping in self.source_config.owner_mappings: | ||
| if mapping["feast_owner_name"] == owner: | ||
| ownership_type_class: OwnershipTypeClass = mapping["ownership_type"] |
There was a problem hiding this comment.
Minor: let's keep the naming consistent here:
datahub_owner_urn
datahub_ownership_type
Also, please add comments to the above Dict field to indicate which keys are required, and which are optional
There was a problem hiding this comment.
I added type hints to reflect that they are optional and in the case they are not passed they will have default values.
| ownership_type_class: OwnershipTypeClass = mapping["ownership_type"] | ||
| return OwnerClass( | ||
| owner=mapping["datahub_owner_urn"], | ||
| type=ownership_type_class, |
There was a problem hiding this comment.
What if a user does not provide an ownership type?
Maybe we have some default here (TECHNICAL owner or something)
There was a problem hiding this comment.
done, thanks
| def _create_owner_association(self, owner: str) -> OwnerClass: | ||
|
|
||
| for mapping in self.source_config.owner_mappings: | ||
| if mapping["feast_owner_name"] == owner: |
There was a problem hiding this comment.
We should add to the comment for the owner mappings argument above that if the user does NOT provide feast_owner_name, no owners will be extracted.
This part is important - many folks will expect that we just plain map the feast owner name into DataHub URN
There was a problem hiding this comment.
I added the comment and also set a default value for datahub_owner_urn.
| @@ -91,6 +96,9 @@ class FeastRepositorySourceConfig(ConfigModel): | |||
| environment: str = Field( | |||
| default=DEFAULT_ENV, description="Environment to use when constructing URNs" | |||
| ) | |||
| owner_mappings: List[Dict[str, str]] = Field( | |||
| default={}, description="Mapping of owner names to owner types" | |||
There was a problem hiding this comment.
As mentioned below, we should have a better description of this argument with mentions of which keys are required, what happens if keys are not specified, etc.
There was a problem hiding this comment.
I added the comment and also added owner_mappings as Optional
| @@ -91,6 +96,9 @@ class FeastRepositorySourceConfig(ConfigModel): | |||
| environment: str = Field( | |||
| default=DEFAULT_ENV, description="Environment to use when constructing URNs" | |||
| ) | |||
| owner_mappings: List[Dict[str, str]] = Field( | |||
There was a problem hiding this comment.
Let's also add 2 additional flags:
enable_owner_extraction: BOOL - If this is disabled, then we NEVER try to map owners. If this is enabled, then owner_mappings is REQUIRED to extract ownership. (We can use something like this for the docs)enable_tag_extraction: BOOL - If this is disabled, then we NEVER try to extract tags.
There was a problem hiding this comment.
The code is already forcing this behavior, I also left comments to reinforce this.
…ub-project#11784) Co-authored-by: John Joyce <john@acryl.io>
Checklist