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

validate permission levels relative to the object #706

Merged
merged 4 commits into from
Jul 6, 2021

Conversation

psg2
Copy link
Contributor

@psg2 psg2 commented Jul 1, 2021

Hi, folks.

This change is so we can validate during the plan relation between different resource attributes which is yet not possible on Terraform other than ConflictsWith.

The idea here is to block resources like:

resource "databricks_permissions" "cluster_permissions" {
  cluster_id = "0000"

  access_control {
    user_name = "me"

    permission_level = "CAN_USE"
  }
}

With an error like this one:

Error: permission_level CAN_USE is not supported with cluster_id objects

  on main.tf line 10, in resource "databricks_permissions" "cluster_permissions":
  10: resource "databricks_permissions" "cluster_permissions" {

References:

@psg2 psg2 changed the title validate permission levels relative to the object DRAFT: validate permission levels relative to the object Jul 1, 2021
@psg2 psg2 changed the title DRAFT: validate permission levels relative to the object validate permission levels relative to the object Jul 1, 2021
@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

Merging #706 (2b35aa5) into master (841eaea) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
- Coverage   83.48%   83.44%   -0.04%     
==========================================
  Files          87       87              
  Lines        7980     7991      +11     
==========================================
+ Hits         6662     6668       +6     
- Misses        840      842       +2     
- Partials      478      481       +3     
Impacted Files Coverage Δ
access/resource_permissions.go 72.24% <100.00%> (-1.49%) ⬇️
sqlanalytics/resource_widget.go 83.58% <0.00%> (+0.74%) ⬆️

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.

@psg2 , great addition. Please see the comment

objectKey string
allowedPermissionLevels []string
}{
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please incorporate with generic permissions mapping:

https://github.com/databrickslabs/terraform-provider-databricks/blob/993bc17c22cd32569fb3f65d071956c9722386a7/access/resource_permissions.go#L230

And perhaps fetch permission levels from API, so that provider won't have to be changed once new permissions are added on the platform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, forgot that we had those on the API. Will do the improvement.

Copy link
Contributor Author

@psg2 psg2 Jul 1, 2021

Choose a reason for hiding this comment

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

However, one question just saw that the API to get cluster permissions for instance requires a cluster_id. I'm thinking if this won't be a problem, since that the CustomizeDiff runs during the plan if I understood correctly there might be the case that the cluster_id is still unresolved and we won't be able to call the API. What are your thoughts here? The same is valid for the other object types apparently.

Copy link
Contributor

Choose a reason for hiding this comment

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

@psg2 you're right. let's then keep them hardcoded. but in permissionsResourceIDFields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nfx
Copy link
Contributor

nfx commented Jul 1, 2021

And we need to incorporate custom diff to couple of other resources, thanks for taking the first step

@psg2
Copy link
Contributor Author

psg2 commented Jul 1, 2021

And we need to incorporate custom diff to couple of other resources, thanks for taking the first step

Do we have issues with those? I can take some others as well.

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.

Code looks good. Have to double check permissions mapping next week

@nfx nfx merged commit c822112 into databricks:master Jul 6, 2021
@nfx nfx mentioned this pull request Jul 14, 2021
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.

None yet

2 participants