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

Exporter: export permissions for databricks_notebook & databricks_directory #1908

Merged
merged 32 commits into from
Feb 24, 2023

Conversation

qili86
Copy link
Contributor

@qili86 qili86 commented Jan 5, 2023

Tested
For notebook,
it generates the permission block in the access.tf, an example is:
resource "databricks_permissions" "notebook_users_qi_li_mckesson_com_tf_idf_test_3534645884600778" { notebook_id = "3534645884600778" access_control { user_name = "XXXXXXXX" (replace the real email) permission_level = "CAN_READ" } }

in import.sh, it generates a new line:
terraform import databricks_permissions.notebook_users_qi_li_mckesson_com_tf_idf_test_3534645884600778 "/notebooks/3534645884600778"

For directory:
it generates the permission block in the access.tf, an example is:
resource "databricks_permissions" "directory_users_qi_li_mckesson_com_directory_test_2864623526846789" { directory_id = "2864623526846789" access_control { user_name = "XXXXXXXX". (replace the real email) permission_level = "CAN_READ" } }

in import.sh, it generates a new line:
terraform import databricks_permissions.directory_users_qi_li_mckesson_com_directory_test_2864623526846789 "/directories/2864623526846789"

This fixes #1850

@qili86 qili86 added the exporter TF configuration generator label Jan 5, 2023
@qili86 qili86 requested review from nfx and alexott January 5, 2023 18:18
@qili86 qili86 changed the title Issue 1850 [ISSUE] Issue with databricks_permissions resource. Permissions are missing on notebooks & directories #1850 Jan 5, 2023
@qili86
Copy link
Contributor Author

qili86 commented Jan 5, 2023

the test fails, will debug more on that

@qili86 qili86 changed the title [ISSUE] Issue with databricks_permissions resource. Permissions are missing on notebooks & directories #1850 Exporter: Issue with databricks_permissions resource. Permissions are missing on notebooks & directories #1850 Jan 5, 2023
@qili86 qili86 changed the title Exporter: Issue with databricks_permissions resource. Permissions are missing on notebooks & directories #1850 Exporter: fix issue with databricks_permissions resource. Permissions are missing on notebooks & directories #1850 Jan 5, 2023
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

see comments. Also, tests are completely missing for new functionality

exporter/importables.go Show resolved Hide resolved
exporter/importables.go Show resolved Hide resolved
exporter/importables.go Show resolved Hide resolved
workspace/resource_notebook.go Show resolved Hide resolved
exporter/importables.go Outdated Show resolved Hide resolved
exporter/importables.go Show resolved Hide resolved
@alexott
Copy link
Contributor

alexott commented Jan 5, 2023

Another issue - you can't expose user's directories or something like /Users as resources - they already exist

@qili86
Copy link
Contributor Author

qili86 commented Jan 5, 2023

Another issue - you can't expose user's directories or something like /Users as resources - they already exist

Can you explain more on this? thanks!

@qili86
Copy link
Contributor Author

qili86 commented Jan 5, 2023

see comments. Also, tests are completely missing for new functionality

will add tests after we finalize the right solution, thanks!

@alexott
Copy link
Contributor

alexott commented Jan 6, 2023

In general, it would be less work if the intended design would be first coordinated/discussed, as I was already starting working on this issue.

To the rest of the comments:

  • Some directories, like, /Users, /, and user's directories need to be either not exported as they are always exist, or should be exported as references to user's home, etc. - like it's already done with reference handling in notebooks, ...
  • Permissions on notebooks & directories need to be exported only if they are different from the parent's resource permissions. In this case we won't generate too many resources and this will solve main issue with the current approach
  • Directories need to be exported only their permissions are different from parent (this could be done differently - either by emitting all directories and then filtering them out later, or by emitting them only if permissions are different)
  • Specific directories could be exported by using -match argument for notebooks.

@qili86
Copy link
Contributor Author

qili86 commented Jan 6, 2023

@alexott , apologies that we don't know that you started working on this issue, if you think it is easier for you to implement this, feel free to close this PR, and change the owner in the following spreadsheet, thank you!

https://docs.google.com/spreadsheets/d/1IA8oBNczSPrt2NmATbY8X0V7JhIpm2oKPTWY_4HfxsM/edit#gid=0

In general, it would be less work if the intended design would be first coordinated/discussed, as I was already starting working on this issue.

To the rest of the comments:

  • Some directories, like, /Users, /, and user's directories need to be either not exported as they are always exist, or should be exported as references to user's home, etc. - like it's already done with reference handling in notebooks, ...
  • Permissions on notebooks & directories need to be exported only if they are different from the parent's resource permissions. In this case we won't generate too many resources and this will solve main issue with the current approach
  • Directories need to be exported only their permissions are different from parent (this could be done differently - either by emitting all directories and then filtering them out later, or by emitting them only if permissions are different)
  • Specific directories could be exported by using -match argument for notebooks.

@qili86 qili86 closed this Jan 22, 2023
@qili86 qili86 reopened this Jan 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2023

Codecov Report

Merging #1908 (eda53ff) into master (f7967ca) will decrease coverage by 0.07%.
The diff coverage is 81.48%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1908      +/-   ##
==========================================
- Coverage   90.34%   90.28%   -0.07%     
==========================================
  Files         136      136              
  Lines       10887    10968      +81     
==========================================
+ Hits         9836     9902      +66     
- Misses        665      674       +9     
- Partials      386      392       +6     
Impacted Files Coverage Δ
exporter/importables.go 89.24% <71.69%> (-1.01%) ⬇️
exporter/util.go 88.96% <100.00%> (+0.44%) ⬆️
workspace/resource_notebook.go 90.20% <100.00%> (+1.23%) ⬆️

@qili86 qili86 requested a review from alexott February 2, 2023 20:39
@nfx
Copy link
Contributor

nfx commented Feb 16, 2023

@qili86 any updates? :)

@qili86
Copy link
Contributor Author

qili86 commented Feb 16, 2023

@qili86 any updates? :)

@alexott, could you do a final review, let me know what else we need to improve, thank you!

@qili86 qili86 requested review from Tagar and nkvuong and removed request for Tagar February 17, 2023 02:30
@nfx nfx requested a review from Tagar February 23, 2023 10:33
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm

@nfx nfx enabled auto-merge (squash) February 24, 2023 16:43
@nfx nfx merged commit 5eb908d into databricks:master Feb 24, 2023
@nfx nfx mentioned this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter TF configuration generator ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ISSUE] Issue with databricks_permissions resource. Permissions are missing on notebooks & directories
5 participants