Copilot commented on code in PR #350:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/350#discussion_r4013050359
##########
cloudstack/resource_cloudstack_role_permission.go:
##########
@@ -205,24 +234,14 @@ func resourceCloudStackRolePermissionDelete(d
*schema.ResourceData, meta interfa
return nil
}
- rolePermissionsByID := make(map[string]*cloudstack.RolePermission)
- for _, rp := range rolePermissions {
- rolePermissionsByID[rp.Id] = rp
- }
-
- used := make(map[string]bool)
- for _, permission := range
rolePermissionSpecs(d.Get("permission").([]interface{})) {
- ruleID := permission.ID
- if ruleID == "" {
- if rp := findMatchingRolePermission(rolePermissions,
permission, used); rp != nil {
- ruleID = rp.Id
- }
- }
- if ruleID == "" || rolePermissionsByID[ruleID] == nil {
+ // Not authoritative: remove only the permissions this resource manages
and
+ // leave anything added outside Terraform in place.
+ desiredPermissions :=
rolePermissionSpecs(d.Get("permission").([]interface{}))
+ for _, rp := range matchCloudStackRolePermissions(rolePermissions,
desiredPermissions) {
Review Comment:
Destroy currently matches only by rule and can delete the wrong permission:
if the state ID is gone but an externally recreated permission has the same
rule, this resource will delete that external permission even though
`authoritative = false` is supposed to preserve undeclared permissions. Prefer
the persisted `permission.ID` when it is present and only use rule matching as
a fallback for ID-less entries, as the previous implementation did.
This issue also appears on line 330 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
