-
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
feat(cli): Add support for custom ownership types to dataproduct CLI #9762
feat(cli): Add support for custom ownership types to dataproduct CLI #9762
Conversation
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.
One comment, but mostly looks good
@@ -332,6 +336,7 @@ def add_owner(urn: str, owner: str, owner_type: str) -> None: | |||
if not urn.startswith("urn:li:dataProduct:"): | |||
urn = f"urn:li:dataProduct:{urn}" | |||
dataproduct_patcher: DataProductPatchBuilder = DataProduct.get_patch_builder(urn) | |||
owner_type, owner_type_urn = validate_ownership_type(owner_type) |
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.
is owner_type_urn
unused here?
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.
Good catch, as I mentioned in description, via this command we can not add custom ownership because we are bound by choices list. Nevertheless the variable should not be left unused, I added it to the OwnerClass
. Thanks!
@hsheth2 All the tests are passing, can we merge the PR? |
The change allows user to specify
urn
of a custom ownership type in thetype
field of the ownership list.The logic is preserved both ways (
from_yaml
andfrom_datahub
) - there are tests in place for that.Additionally the PR contains fix to some problems in
_patch_ownership
function:type
defined thenid
was set wrongly - now it is fixedI decided to keep predefined, enum-based choice list in
dataproduct add_owner
cli command - if support of ownership type urn is needed the change is required only within theadd_owner
function ofdataproduct_cli.py
- I can adjust it in this PR if needed.