add optional config for ownership type in ownership transforms #3159
Conversation
|
@shirshanka : I'm not a python expert, so I would appreciate your feedback on this (code, naming, etc) before going on and update docs. Thanks! |
| @@ -112,6 +113,21 @@ def make_ml_model_group_urn(platform: str, group_name: str, env: str) -> str: | |||
| ) | |||
|
|
|||
|
|
|||
| def check_ownership_type(ownership_type: Optional[str]) -> str: | |||
There was a problem hiding this comment.
Name of the function is somewhat misleading. Looking at a check_* function I would assume a boolean return type. Maybe use is_valid_ownership_type and actually return a boolean? That boolean can be used in the transformer.
| return ownership_type | ||
| else: | ||
| logger.warning(f"invalid ownership type value: {ownership_type}") | ||
| return OwnershipTypeClass.DATAOWNER |
There was a problem hiding this comment.
Having a default value here can cause confusion when using this function.
| @@ -60,14 +59,18 @@ def transform_one(self, mce: MetadataChangeEventClass) -> MetadataChangeEventCla | |||
| class SimpleDatasetOwnershipConfig(ConfigModel): | |||
| owner_urns: List[str] | |||
| default_actor: str = builder.make_user_urn("etl") | |||
| ownership_type: Optional[str] = None | |||
There was a problem hiding this comment.
Might be better to add that default value here instead of None. This will keep things backward compatible which seemed to be your intention by sending that value back
|
Thanks @aseembansal-gogo for the review. I have refactored the code, mainly moving default value close to the transformer itself. My biggest concern now is on having the list of valid ownership types for the validation because it needs to be updated when new types are added. Alternatively I could skip that validation in the transformer and just let the MCE event fail later, in the GMS API. WDYT? |
|
@aseembansal-gogo @shirshanka may I ask you to review again after last update? thanks |
There was a problem hiding this comment.
This LGTM.
I think there could be a more sophisticated version of this config like, which we can do in a follow up PR. We also would need to auto-upgrade old style config to new style in that case.
owners:
users:
- id: jdoe
type: DATAOWNER
- id: bsmith
type: DEVELOPER
groups:
-id: security
type: STAKEHOLDER
This is adding the ability to specify the ownership type in the ownership transformations.
DATAOWNERis the default value if not given or if an unexpected value is given.Checklist