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.

Sign up for GitHub

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

Jump to bottom

Fixing assessment throwing an error when the owner of an object is empty #485

Merged

nfx merged 17 commits into main from fix_empty_owners

Oct 27, 2023

Contributor

william-conti commented Oct 20, 2023

Fixes #73

By default, all objects without owners will have their created_by column persisted as an empty string instead of null/None for the sake of backward compatibility.

It also adds Integration tests for the SQLBackend in order to harden the code

william-conti requested a review from a team

October 20, 2023 17:52

william-conti had a problem deploying to account-admin

October 20, 2023 17:52

— with

GitHub Actions Failure

william-conti changed the title ~~Fixing assessment throwing an error when the owner ob an object is empty~~ Fixing assessment throwing an error when the owner of an object is empty

codecov bot commented Oct 20, 2023 •

edited

Loading

Codecov Report

Merging #485 (6dbd3dc) into main (726fc6c) will increase coverage by 0.30%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##             main     #485      +/-   ##
==========================================
+ Coverage   80.31%   80.61%   +0.30%     
==========================================
  Files          31       31              
  Lines        3220     3240      +20     
  Branches      617      625       +8     
==========================================
+ Hits         2586     2612      +26     
+ Misses        489      479      -10     
- Partials      145      149       +4

Files	Coverage Δ
...rc/databricks/labs/ucx/workspace_access/generic.py	`91.32% <100.00%> (+0.50%)`	⬆️
src/databricks/labs/ucx/assessment/crawlers.py	`86.83% <95.23%> (+1.60%)`	⬆️

nfx requested changes

View reviewed changes

Collaborator

nfx left a comment

Where's the change for GenericPermissionsSupport? :)

src/databricks/labs/ucx/assessment/crawlers.py Outdated Show resolved Hide resolved

tests/unit/assessment/test_assessment.py Outdated Show resolved Hide resolved

william-conti added 7 commits

October 23, 2023 16:01


          Fixing assessment throwing an error when the owner is empty

54ae94b


          making code forward compatible

df7fca0


          making code forward compatible

c78b185


          fixing test name

e4cd5a1


          removing irrelevant lines


          fixing irrelevant changes

06e2924


          adding code in generic

d699df4

william-conti force-pushed the fix_empty_owners branch from 990ead0 to d699df4 Compare

October 24, 2023 18:28

william-conti had a problem deploying to account-admin

October 24, 2023 18:28

— with

GitHub Actions Failure

william-conti had a problem deploying to account-admin

October 25, 2023 06:48

— with

GitHub Actions Failure


          adding more nullable fields

1db294b

william-conti force-pushed the fix_empty_owners branch from 26eb6f2 to 1db294b Compare

October 25, 2023 11:28

william-conti had a problem deploying to account-admin

October 25, 2023 11:28

— with

GitHub Actions Failure

fix

william-conti had a problem deploying to account-admin

October 25, 2023 11:32

— with

GitHub Actions Failure

This was referenced Oct 25, 2023

Assessment task failed in pipeline crawler #504

Closed

Assessment task failed in Jobs crawler #503

Closed

nfx requested changes

View reviewed changes

Collaborator

nfx left a comment

integration tests must pass

src/databricks/labs/ucx/assessment/crawlers.py Outdated

                   success: int
                   failures: str
+                  cluster_id: str = None

Collaborator

nfx Oct 25, 2023

cluster, job, pipeline IDs - they cannot be done. it makes no sense ;)

src/databricks/labs/ucx/assessment/crawlers.py Outdated

+                              created_by=gis.created_by,
+                              enabled=gis.enabled,
+                              success=1,
+                              failures="",

Collaborator

nfx Oct 25, 2023

Suggested change

      
                            failures="",
          
                            failures="[]",

src/databricks/labs/ucx/assessment/crawlers.py Outdated

+                              pipeline_name=pipeline.name,
+                              creator_name=pipeline.creator_user_name,
+                              success=1,
+                              failures="",

Collaborator

nfx Oct 25, 2023

Suggested change

      
                            failures="",
          
                            failures="[]",

src/databricks/labs/ucx/assessment/crawlers.py Outdated

+                              cluster_name=cluster.cluster_name,
+                              creator=cluster.creator_user_name,
+                              success=1,
+                              failures="",

Collaborator

nfx Oct 25, 2023

Suggested change

      
                            failures="",
          
                            failures="[]",

src/databricks/labs/ucx/assessment/crawlers.py Outdated

+                              job_name=job.settings.name,
+                              creator=job.creator_user_name,
+                              success=1,
+                              failures="",

Collaborator

nfx Oct 25, 2023

Suggested change

      
                            failures="",
          
                            failures="[]",


          last fixes

3575a67

william-conti had a problem deploying to account-admin

October 26, 2023 17:07

— with

GitHub Actions Error


          Merge branch 'main' into fix_empty_owners

7961dba

william-conti had a problem deploying to account-admin

October 26, 2023 17:09

— with

GitHub Actions Failure


          fixing bugs'

3da5715

william-conti had a problem deploying to account-admin

October 27, 2023 07:56

— with

GitHub Actions Failure


          removing irrelevant changes

625a7bd

william-conti had a problem deploying to account-admin

October 27, 2023 08:01

— with

GitHub Actions Failure


          last changes

william-conti had a problem deploying to account-admin

October 27, 2023 08:03

— with

GitHub Actions Failure


          renaming

95d5031

william-conti temporarily deployed to account-admin

October 27, 2023 08:05

— with

GitHub Actions Inactive

nfx requested changes

View reviewed changes

Collaborator

nfx left a comment

could you add a unit test for the warning by codecov? :)

src/databricks/labs/ucx/workspace_access/generic.py Show resolved Hide resolved

tests/unit/assessment/test_assessment.py Show resolved Hide resolved

tests/unit/workspace_access/test_generic.py Outdated

+                  assert tasks[0] == Permissions(
+                      object_id="1234",
+                      object_type="clusters",
+                      raw='{"access_control_list": [{"all_permissions": ['

Collaborator

nfx Oct 27, 2023

don't test for serialized JSON, because the order of fields may change between invocations.


          adding tests

67aad27

william-conti had a problem deploying to account-admin

October 27, 2023 10:54

— with

GitHub Actions Failure


          removing useless comment

6dbd3dc

william-conti had a problem deploying to account-admin

October 27, 2023 10:57

— with

GitHub Actions Failure

william-conti had a problem deploying to account-admin

October 27, 2023 11:46

— with

GitHub Actions Failure

william-conti had a problem deploying to account-admin

October 27, 2023 13:55

— with

GitHub Actions Failure

william-conti temporarily deployed to account-admin

October 27, 2023 14:40

— with

GitHub Actions Inactive

nfx approved these changes

View reviewed changes

nfx merged commit ebf66ad into main

nfx added the breaking change label

nfx deleted the fix_empty_owners branch

October 28, 2023 15:08

nfx added a commit that referenced this pull request


          Release v0.6.0

5aa1587

**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)).

nfx mentioned this pull request

Release v0.6.0 #598

Merged

nfx added a commit that referenced this pull request


          Release v0.6.0 (#598)

b4dab33

**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)).

pritishpai pushed a commit that referenced this pull request


          Release v0.6.0 (#598)

301a4e3

**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)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet