-
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
fix(ingest/tableau): graceful handling of get all datasources failure… #8406
fix(ingest/tableau): graceful handling of get all datasources failure… #8406
Conversation
652fed1
to
47ea73c
Compare
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.
A couple refactor requests but LGTM
) | ||
continue | ||
self.datasource_project_map[ds.id] = ds.project_id | ||
self.report.get_all_datasources_query_failed = False |
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.
This looks like a no-op. Either way we should set the default to None
or just remove this statement
@@ -1304,6 +1331,30 @@ def _get_published_datasource_project_luid(self, ds): | |||
|
|||
return None | |||
|
|||
def _query_published_datasource_for_project_luid(self, ds: dict) -> 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.
Can we pass in ds[LUID]
rather than ds
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.
addressed in followup PR
container_key = self._get_datasource_container_key( | ||
datasource, workbook, is_embedded_ds | ||
) |
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.
Nice
…, add fallback
Also adds TableauSourceReport to track failures when parsing tableau response.
Checklist