Skip to content
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

Check if permissions are up to date #421

Merged
merged 19 commits into from
Oct 12, 2023
Merged

Conversation

william-conti
Copy link
Contributor

Attempt to fix #36

Goal of this PR is to add a validation in each class that implements the Crawler and Applier class.
This validation step will verify that a permissions that has been applied to an object is correct, if it's not the case, it attemps to re-apply this permission multiple times

@william-conti william-conti requested review from a team October 10, 2023 08:38
@william-conti william-conti marked this pull request as draft October 10, 2023 08:38
@larsgeorge-db
Copy link
Contributor

How can this happen in the first place? Could you add some info around this use-case? Or edge case?

src/databricks/labs/ucx/workspace_access/base.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/workspace_access/generic.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #421 (b75ce1b) into main (1f07fff) will increase coverage by 0.17%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##             main     #421      +/-   ##
==========================================
+ Coverage   79.55%   79.72%   +0.17%     
==========================================
  Files          31       31              
  Lines        3116     3152      +36     
  Branches      599      611      +12     
==========================================
+ Hits         2479     2513      +34     
  Misses        494      494              
- Partials      143      145       +2     
Files Coverage Δ
...rc/databricks/labs/ucx/workspace_access/generic.py 82.79% <100.00%> (+1.61%) ⬆️
src/databricks/labs/ucx/workspace_access/redash.py 84.94% <100.00%> (+1.22%) ⬆️
src/databricks/labs/ucx/workspace_access/scim.py 96.42% <88.23%> (-3.58%) ⬇️

Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor comments before we can merge this

src/databricks/labs/ucx/workspace_access/scim.py Outdated Show resolved Hide resolved
@william-conti william-conti force-pushed the fix_multithreaded_group_migration branch from 8fbd2fd to caddf4c Compare October 11, 2023 11:05
@william-conti william-conti changed the title [WIP] Check if permissions are up to date Check if permissions are up to date Oct 11, 2023
@william-conti william-conti marked this pull request as ready for review October 11, 2023 13:49
@william-conti william-conti force-pushed the fix_multithreaded_group_migration branch 2 times, most recently from d22adfe to 05206af Compare October 11, 2023 18:00
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few small changes

tests/integration/workspace_access/test_generic.py Outdated Show resolved Hide resolved
tests/integration/workspace_access/test_redash.py Outdated Show resolved Hide resolved
tests/integration/workspace_access/test_redash.py Outdated Show resolved Hide resolved
tests/integration/workspace_access/test_redash.py Outdated Show resolved Hide resolved
tests/integration/workspace_access/test_scim.py Outdated Show resolved Hide resolved
@william-conti william-conti force-pushed the fix_multithreaded_group_migration branch from b005248 to 6f08b60 Compare October 12, 2023 15:57
@william-conti william-conti force-pushed the fix_multithreaded_group_migration branch from 8706168 to 47a8bf1 Compare October 12, 2023 16:19
@nfx nfx disabled auto-merge October 12, 2023 17:47
@nfx nfx merged commit aca7707 into main Oct 12, 2023
5 checks passed
@nfx nfx deleted the fix_multithreaded_group_migration branch October 12, 2023 17:48
@nfx nfx mentioned this pull request Oct 18, 2023
FastLee pushed a commit that referenced this pull request Oct 25, 2023
Attempt to fix #36

Goal of this PR is to add a validation in each class that implements the
Crawler and Applier class.
This validation step will verify that a permissions that has been
applied to an object is correct, if it's not the case, it attemps to
re-apply this permission multiple times
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validation Suite & Report to confirm what is available in inventory vs migrated
3 participants