Copilot commented on code in PR #350:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/350#discussion_r4013801555
##########
cloudstack/resource_cloudstack_role_permission.go:
##########
@@ -44,6 +45,13 @@ func resourceCloudStackRolePermission() *schema.Resource {
Read: resourceCloudStackRolePermissionRead,
Update: resourceCloudStackRolePermissionUpdate,
Delete: resourceCloudStackRolePermissionDelete,
+ // Reject duplicate rules at plan time. Doing it only in Update
is too late:
+ // SDKv2 merges the planned permission list into state before
Update runs, so
+ // an error raised there persists the invalid list even though
CloudStack was
+ // never called.
+ CustomizeDiff: func(_ context.Context, d *schema.ResourceDiff,
_ interface{}) error {
+ return
validateUniqueRolePermissionRules(rolePermissionSpecs(d.Get("permission").([]interface{})))
+ },
Review Comment:
`CustomizeDiff` runs during planning, where `permission` entries can be
unknown (e.g., derived from computed values). `rolePermissionSpecs` assumes
each list element is a `map[string]interface{}` and will panic if an element is
unknown/non-map. Consider making the duplicate-rule validation tolerant of
unknowns at plan time (e.g., extract only known `rule` strings, and skip
validation when rules are unknown) so planning never crashes the provider.
##########
cloudstack/resource_cloudstack_role_permission.go:
##########
@@ -205,24 +239,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) {
+ if rp == nil {
continue
}
- used[ruleID] = true
- if err := deleteCloudStackRolePermission(cs, ruleID); err !=
nil {
+ if err := deleteCloudStackRolePermission(cs, rp.Id); err != nil
{
Review Comment:
The comment says “Not authoritative”, but this delete behavior is
unconditional and the function does not branch on `authoritative`. Since `Read`
now surfaces external permissions into state when authoritative is enabled
(with empty IDs), this delete path can end up deleting permissions that were
created out-of-band as well. Either (mandatory) update the comment to reflect
the actual behavior, or (optional) branch on `authoritative` to make the
behavior explicitly match the intended semantics.
--
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]
