-
Notifications
You must be signed in to change notification settings - Fork 3k
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(ingest/snowflake): support email_as_user_identifier for queries v2 #12219
feat(ingest/snowflake): support email_as_user_identifier for queries v2 #12219
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.
LGTM
Minor suggestions
@@ -225,6 +227,9 @@ def is_allowed_table(self, name: str) -> bool: | |||
def get_workunits_internal( | |||
self, | |||
) -> Iterable[MetadataWorkUnit]: | |||
with self.report.users_fetch_timer: | |||
users = self.fetch_users() |
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.
It can be tough to figure out what's in the dictionary (key and value) especially when variables go through different methods. To help, I suggest using a more descriptive name or a type alias to clearly describe the content.
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.
looks better now ?
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.
yes
"email_as_user_identifier": True, | ||
"email_domain": "example.com", |
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.
do we miss the case below? it that makes sense
email_as_user_identifier: True
email_domain: None
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.
That's true -> I'll add that to test -> but in this case - we'd do best effort and then if email still can't be formed then use username for urn.
Checklist