Copilot commented on code in PR #351:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/351#discussion_r4034103566
##########
cloudstack/resource_cloudstack_instance.go:
##########
@@ -794,6 +856,35 @@ func resourceCloudStackInstanceUpdate(d
*schema.ResourceData, meta interface{})
}
}
+ // Update security groups once, even when switching between
names and IDs.
+ if d.HasChange("security_group_ids") ||
d.HasChange("security_group_names") {
+ p :=
cs.VirtualMachine.NewUpdateVirtualMachineParams(d.Id())
+ ids := d.Get("security_group_ids").(*schema.Set)
+ names := d.Get("security_group_names").(*schema.Set)
+
Review Comment:
The new in-place security-group path has no acceptance coverage in
`resource_cloudstack_instance_test.go`: there are no instance configurations
using either security-group attribute, nor checks that a running VM is
restarted or a stopped VM remains stopped. Please add update steps covering
IDs, names, clearing/switching the mutually exclusive forms, and both initial
VM states so the parameter selection and lifecycle guarantees in this change
are exercised.
--
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]
