-
Notifications
You must be signed in to change notification settings - Fork 402
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
validate permission levels relative to the object #706
Conversation
Codecov Report
@@ 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
|
There was a problem hiding this 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
access/resource_permissions.go
Outdated
objectKey string | ||
allowedPermissionLevels []string | ||
}{ | ||
{ |
There was a problem hiding this comment.
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:
And perhaps fetch permission levels from API, so that provider won't have to be changed once new permissions are added on the platform
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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. |
There was a problem hiding this 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
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:
With an error like this one:
References: