-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add history log encoder for dashboards #3424
base: main
Are you sure you want to change the base?
Conversation
f417e8d
to
3bf9a07
Compare
❌ 100/101 passed, 9 flaky, 1 failed, 7 skipped, 5h24m27s total ❌ test_redash_dashboard_ownership_is_me: AssertionError: Invalid owner for dashboard: Dashboard(id='b7199ffc-e28c-41e7-8bbf-edf3e64cd63d', name='ucx_D7n9L_ra78a5a6a0', parent='folders/3865756826903956', query_ids=['1cb92323-637b-487b-94ef-22724c98cab4'], tags=['original_dashboard_tag'], creator_id='481119220561874') (4m13.495s)
Flaky tests:
Running from acceptance #7806 |
0386b5c
to
54c4442
Compare
The directionality will be the other way around: code points to used tables
78e563b
to
8beff2c
Compare
02282a0
to
7a50eff
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.
Small Changes
@@ -410,7 +413,7 @@ def _maybe_direct_owner(self, record: Dashboard) -> str | None: | |||
def _get_user_name(self, user_id: str) -> str | None: | |||
try: | |||
user = self._ws.users.get(user_id) | |||
return user.display_name or user.user_name |
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.
Are we sure about that?
@@ -266,6 +267,10 @@ def udfs_crawler(self) -> UdfsCrawler: | |||
def udf_ownership(self) -> UdfOwnership: | |||
return UdfOwnership(self.administrator_locator) | |||
|
|||
@cached_property |
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.
Should we consider combining all ownerships to a single method?
logger.warning(f"Cannot retrieve status for: {path}") | ||
return None | ||
if not (object_info.object_id and object_info.object_type): | ||
return None | ||
object_id = str(object_info.object_id) | ||
match object_info.object_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.
Change to return object_info.object_type.lower(), object_id
self._inventory_database = inventory_database | ||
self._used_tables_crawlers = used_tables_crawlers | ||
|
||
def append_inventory_snapshot(self, snapshot: Iterable[Dashboard]) -> 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.
def append_inventory_snapshot(self, snapshot: Iterable[Dashboard]) -> None: | |
def append_dashboards(self, snapshot: dashboards[Dashboard]) -> None: |
|
||
def _get_query_problems(self) -> DashboardIdToFailuresType: | ||
index = collections.defaultdict(list) | ||
for row in self._sql_backend.fetch( |
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.
Shouldn't we use a crawler for that?
|
||
def _get_tables_failures(self) -> DashboardIdToFailuresType: | ||
table_failures = {} | ||
for row in self._sql_backend.fetch( |
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.
Same as above
Changes
Add history log encoder for dashboards
Linked issues
Resolves #3368
Resolves #3369
Functionality
experimental-migration-progress
Tests