-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix(ingestion/looker): ingest looks not part of dashboard #8140
fix(ingestion/looker): ingest looks not part of dashboard #8140
Conversation
metadata-ingestion/src/datahub/ingestion/source/looker/looker_config.py
Outdated
Show resolved
Hide resolved
) | ||
) | ||
# Add soft deleted looks | ||
looks.extend(self.search_looks(fields=fields, deleted=True)) |
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.
why are we including soft-deleted looks?
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.
We have config to include soft deleted Dashboards source_config.include_deleted
.
I put a condition on line 214 to add soft deleted looks if include_deleted
is set
metadata-ingestion/src/datahub/ingestion/source/looker/looker_source.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/looker/looker_source.py
Outdated
Show resolved
Hide resolved
self.client_stats.search_looks_calls += 1 | ||
return list( | ||
self.client.search_looks( | ||
fields=self.__fields_mapper(fields), |
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.
just an fyi - python convention is to use a single _
for private methods. the double underscore __method
has a different meaning and should be avoided
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.
noted...
metadata-ingestion/src/datahub/ingestion/source/looker/looker_source.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/looker/looker_source.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/looker/looker_source.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/looker/looker_source.py
Outdated
Show resolved
Hide resolved
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.
few nits but otherwise lgtm
@@ -135,7 +135,8 @@ class LookerDashboardSourceConfig( | |||
description="Patterns for selecting chart ids that are to be included", | |||
) | |||
include_deleted: bool = Field( | |||
False, description="Whether to include deleted dashboards." | |||
False, | |||
description="Whether to include deleted dashboards and deleted independent looks.", |
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.
description="Whether to include deleted dashboards and deleted independent looks.", | |
description="Whether to include deleted dashboards and looks.", |
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.
done
@@ -1196,6 +1203,8 @@ def extract_independent_looks(self) -> Iterable[MetadataWorkUnit]: | |||
dashboard_element=dashboard_element | |||
) | |||
|
|||
self.reporter.report_stage_end("extract_independent_looks") |
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.
The indentation is off 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.
done
No description provided.