Running apply permission task before assessment should display message #487

HariGS-DB · 2023-10-21T09:21:45Z

If the apply permission task is run before the assessment task, it crashes. This PR checks if the permissions table (which is required for the apply group permission) is present if not raise a Databricks error exception

codecov · 2023-10-21T09:23:41Z

Codecov Report

Merging #487 (9bef3e6) into main (334c9b8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   80.25%   80.27%   +0.01%     
==========================================
  Files          31       31              
  Lines        3206     3209       +3     
  Branches      620      621       +1     
==========================================
+ Hits         2573     2576       +3     
  Misses        486      486              
  Partials      147      147

Files	Coverage Δ
...rc/databricks/labs/ucx/workspace_access/manager.py	`82.35% <100.00%> (+0.53%)`	⬆️

src/databricks/labs/ucx/workspace_access/manager.py

nfx · 2023-10-21T13:49:52Z

src/databricks/labs/ucx/workspace_access/manager.py

@@ -148,6 +149,13 @@ def _save(self, items: list[Permissions]):

    def load_all(self) -> list[Permissions]:
        logger.info(f"Loading inventory table {self._full_name}")
+        if self._table not in [d.tableName for d in self._fetch(f"SHOW TABLES IN {self._catalog}.{self._schema}")]:


This is a wrong condition - tables are created by installer now. Check for the presence of records

And what user would be doing with this message? :)

@nfx I thought the installer only creates the job. Its the setup schema in the assessment job which creates the schema first and each task create the tables

@HariGS-DB He meant to say "soon it will" :) See https://github.com/databrickslabs/ucx/pull/474/files#diff-67e29ef6d5123a708ed3144d670ba229c7d64995aae2d58f2ac9f7b69e6c88d5R106 in PR #474

i see. understood. will add the changes and push it

I have done the changes to look for table rows and updated the error msg to be more descriptive. I noticed the actual RunTimeBackend fetch returns a list(even though the func signature says iter) whereas the MockBackend returns an iter

nfx · 2023-10-22T09:05:22Z

src/databricks/labs/ucx/workspace_access/manager.py

@@ -148,6 +148,12 @@ def _save(self, items: list[Permissions]):

    def load_all(self) -> list[Permissions]:
        logger.info(f"Loading inventory table {self._full_name}")
+        if next(iter(self._fetch(f"SELECT COUNT(*) as cnt FROM {self._full_name}")))[0] == 0:


What does next(iter()) do?..

added by ruff

HariGS-DB · 2023-10-23T09:29:21Z

@larsgeorge-db please could approve this PR. approved by @nfx

#487) If the apply permission task is run before the assessment task, it crashes. This PR checks if the permissions table (which is required for the apply group permission) is present if not raise a Databricks error exception

**Breaking changes** (existing installations need to reinstall UCX and re-run assessment jobs) * Switched local group migration component to rename groups instead of creating backup groups ([#450](#450)). * Mitigate permissions loss in Table ACLs by folding grants belonging to the same principal, object id and object type together ([#512](#512)). **New features** * Added support for the experimental Databricks CLI launcher ([#517](#517)). * Added support for external Hive Metastores including AWS Glue ([#400](#400)). * Added more views to assessment dashboard ([#474](#474)). * Added rate limit for creating backup group to increase stability ([#500](#500)). * Added deduplication for mount point list ([#569](#569)). * Added documentation to describe interaction with external Hive Metastores ([#473](#473)). * Added failure injection for job failure message propagation ([#591](#591)). * Added uniqueness in the new warehouse name to avoid conflicts on installation ([#542](#542)). * Added a global init script to collect Hive Metastore lineage ([#513](#513)). * Added retry set/update permissions when possible and assess the changes in the workspace ([#519](#519)). * Use `~/.ucx/state.json` to store the state of both dashboards and jobs ([#561](#561)). **Bug fixes** * Fixed handling for `OWN` table permissions ([#571](#571)). * Fixed handling of keys with and without values. ([#514](#514)). * Fixed integration test failures related to concurrent group delete ([#584](#584)). * Fixed issue with workspace listing process on None type `object_type` ([#481](#481)). * Fixed missing group entitlement migration bug ([#583](#583)). * Fixed entitlement application for account-level groups ([#529](#529)). * Fixed assessment throwing an error when the owner of an object is empty ([#485](#485)). * Fixed installer to migrate between different configuration file versions ([#596](#596)). * Fixed cluster policy crawler to be aware of deleted policies ([#486](#486)). * Improved error message for not null constraints violated ([#532](#532)). * Improved integration test resiliency ([#597](#597), [#594](#594), [#586](#586)). * Introduced Safer access to workspace objects' properties. ([#530](#530)). * Mitigated permissions loss in Table ACLs by running appliers with single thread ([#518](#518)). * Running apply permission task before assessment should display message ([#487](#487)). * Split integration tests from blocking the merge queue ([#496](#496)). * Support more than one dashboard per step ([#472](#472)). * Update databricks-sdk requirement from ~=0.11.0 to ~=0.12.0 ([#505](#505)). * Update databricks-sdk requirement from ~=0.12.0 to ~=0.13.0 ([#575](#575)).

fixed issue on missing permissions table and raise righ t error

a3837a0

HariGS-DB requested a review from a team October 21, 2023 09:21

HariGS-DB linked an issue Oct 21, 2023 that may be closed by this pull request

Running jobs out of order does not give a clear error message #232

Closed

HariGS-DB had a problem deploying to account-admin October 21, 2023 09:21 — with GitHub Actions Failure

nfx requested changes Oct 21, 2023

View reviewed changes

src/databricks/labs/ucx/workspace_access/manager.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/workspace_access/manager.py Outdated Show resolved Hide resolved

changed error message

7013c20

HariGS-DB had a problem deploying to account-admin October 21, 2023 14:45 — with GitHub Actions Failure

HariGS-DB added bug migrate/groups Corresponds to Migrate Groups Step of go/uc/upgrade labels Oct 21, 2023

HariGS-DB temporarily deployed to account-admin October 21, 2023 16:21 — with GitHub Actions Inactive

nfx reviewed Oct 21, 2023

View reviewed changes

changed error message

cbfa74e

HariGS-DB had a problem deploying to account-admin October 21, 2023 17:08 — with GitHub Actions Failure

changed table check to count check

2e1d274

HariGS-DB had a problem deploying to account-admin October 21, 2023 22:42 — with GitHub Actions Error

fmting

d8257a6

HariGS-DB had a problem deploying to account-admin October 21, 2023 22:42 — with GitHub Actions Failure

HariGS-DB had a problem deploying to account-admin October 21, 2023 23:51 — with GitHub Actions Failure

nfx reviewed Oct 22, 2023

View reviewed changes

removing ruff formatting

9bef3e6

HariGS-DB temporarily deployed to account-admin October 22, 2023 12:18 — with GitHub Actions Inactive

nfx approved these changes Oct 22, 2023

View reviewed changes

HariGS-DB added the ready to merge label Oct 22, 2023

HariGS-DB enabled auto-merge October 23, 2023 08:18

larsgeorge-db changed the title ~~running apply permission task before assessment should display right msg~~ Running apply permission task before assessment should display message Oct 23, 2023

HariGS-DB added this pull request to the merge queue Oct 23, 2023

larsgeorge-db approved these changes Oct 23, 2023

View reviewed changes

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 23, 2023

HariGS-DB added this pull request to the merge queue Oct 23, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 23, 2023

nfx merged commit a817e0a into main Oct 23, 2023

nfx deleted the bug/migrate_group_error branch October 23, 2023 13:55

nfx mentioned this pull request Nov 17, 2023

Release v0.6.0 #598

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Running apply permission task before assessment should display message #487

Running apply permission task before assessment should display message #487

HariGS-DB commented Oct 21, 2023

codecov bot commented Oct 21, 2023 •

edited

Loading

nfx Oct 21, 2023

nfx Oct 21, 2023

HariGS-DB Oct 21, 2023

larsgeorge-db Oct 21, 2023

HariGS-DB Oct 21, 2023

HariGS-DB Oct 21, 2023

nfx Oct 22, 2023

HariGS-DB Oct 22, 2023

HariGS-DB commented Oct 23, 2023

Running apply permission task before assessment should display message #487

Running apply permission task before assessment should display message #487

Conversation

HariGS-DB commented Oct 21, 2023

codecov bot commented Oct 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HariGS-DB commented Oct 23, 2023

codecov bot commented Oct 21, 2023 •

edited

Loading