Fix #16573: get table owners for databaricks & unitycatalog tables #17282
Conversation
github-actions
bot
added
Ingestion
safe to test
ingestion/src/metadata/ingestion/source/database/databricks/metadata.py
Outdated
Show resolved
Hide resolved
|
|
||
| def _check_if_email(self, email_id: str) -> bool: | ||
| """check if email string is valid""" | ||
| email_pattern = r"^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+$" |
There was a problem hiding this comment.
do we need to do this here? Seems like a more generic check rather than keeping it only for databricks. How are we solving it for other systems such as postgres where usernames are not necessarily emails?
There was a problem hiding this comment.
In the databricks, we get owner name in both name/email type. So that I have applied here to filter out based on email first and then name.
While in other db/dashboard connection we have confirm that we get owner's email(from api/inspector method) and we're passing directly to get reference.
domodb, iceberg, metabase.
I think we can put this method in DatabaseServiceSource or DashboardServiceSource & check with valid email whenever we're not sure. WDYT ?
There was a problem hiding this comment.
- let's put this method in a common place. maybe https://github.com/open-metadata/OpenMetadata/blob/main/ingestion/src/metadata/utils/helpers.py? I don't love it but we don't have a better structure atm.
- let's try to use pydantic emailStr instead of regex
There was a problem hiding this comment.
I believe we are already installing the extra, if not OK to add it. better than a regex
harshsoni2024
force-pushed
the
issue-16573
branch
from
a167680
to
2c3606b
Compare
harshsoni2024
changed the title
| try: | ||
| owner_email = EmailStr._validate(owner) | ||
| owner_ref = self.metadata.get_reference_by_email(email=owner_email) | ||
| except Exception: |
There was a problem hiding this comment.
- we dont need to catch ANY exception here no? only the one triggered by wrong strings inside
EmailStr - also, I don't think we need to run
._validate. You might just be able to doEmailStr(<string>)and see if it blows up or not
There was a problem hiding this comment.
- I checked with EmailStr(string) but it's not working in that way (maybe with v2 it's changed)
change
harshsoni2024
force-pushed
the
issue-16573
branch
from
ae7eb5a
to
b708c9d
Compare
|
Describe your changes:
Fix #16573
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>