Copilot commented on code in PR #323:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/323#discussion_r3795914216
##########
cloudstack/resource_cloudstack_instance.go:
##########
@@ -681,24 +681,24 @@ func resourceCloudStackInstanceUpdate(d
*schema.ResourceData, meta interface{})
}
- // Check if the service offering is changed and if so, update
the offering
+ // Check if the service offering is changed and if so, scale
the VM
if d.HasChange("service_offering") {
- log.Printf("[DEBUG] Service offering changed for %s,
starting update", name)
+ log.Printf("[DEBUG] Service offering changed for %s,
starting scale", name)
// Retrieve the service_offering ID
serviceofferingid, e := retrieveID(cs,
"service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
- // Create a new parameter struct
- p :=
cs.VirtualMachine.NewChangeServiceForVirtualMachineParams(d.Id(),
serviceofferingid)
+ // Create a new parameter struct for scaling
+ p :=
cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), serviceofferingid)
- // Change the service offering
- _, err =
cs.VirtualMachine.ChangeServiceForVirtualMachine(p)
+ // Scale the VM to the new service offering
+ _, err = cs.VirtualMachine.ScaleVirtualMachine(p)
if err != nil {
return fmt.Errorf(
- "Error changing the service offering
for instance %s: %s", name, err)
+ "Error scaling the service offering for
instance %s: %s", name, err)
Review Comment:
The new error message reads like the *service offering* is being scaled, but
the operation is scaling the VM to a different service offering. Including the
target service offering (name/id) will make failures easier to debug.
##########
cloudstack/resource_cloudstack_instance.go:
##########
@@ -681,24 +681,24 @@ func resourceCloudStackInstanceUpdate(d
*schema.ResourceData, meta interface{})
}
- // Check if the service offering is changed and if so, update
the offering
+ // Check if the service offering is changed and if so, scale
the VM
if d.HasChange("service_offering") {
- log.Printf("[DEBUG] Service offering changed for %s,
starting update", name)
+ log.Printf("[DEBUG] Service offering changed for %s,
starting scale", name)
// Retrieve the service_offering ID
serviceofferingid, e := retrieveID(cs,
"service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
- // Create a new parameter struct
- p :=
cs.VirtualMachine.NewChangeServiceForVirtualMachineParams(d.Id(),
serviceofferingid)
+ // Create a new parameter struct for scaling
+ p :=
cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), serviceofferingid)
- // Change the service offering
- _, err =
cs.VirtualMachine.ChangeServiceForVirtualMachine(p)
+ // Scale the VM to the new service offering
+ _, err = cs.VirtualMachine.ScaleVirtualMachine(p)
Review Comment:
This change scales only when `service_offering` changes, but issue #273 also
calls out dynamic scaling when compute-related VM details change (e.g.,
`details.cpuNumber`, `details.cpuSpeed`, `details.memory`). As written,
`details` updates are still handled later via `UpdateVirtualMachine`, which
won’t apply CPU/memory scaling; consider detecting those detail key changes and
invoking `ScaleVirtualMachine` (with `details` populated) even when the service
offering name/id itself is unchanged.
##########
cloudstack/resource_cloudstack_instance.go:
##########
@@ -681,24 +681,24 @@ func resourceCloudStackInstanceUpdate(d
*schema.ResourceData, meta interface{})
}
- // Check if the service offering is changed and if so, update
the offering
+ // Check if the service offering is changed and if so, scale
the VM
if d.HasChange("service_offering") {
- log.Printf("[DEBUG] Service offering changed for %s,
starting update", name)
+ log.Printf("[DEBUG] Service offering changed for %s,
starting scale", name)
// Retrieve the service_offering ID
serviceofferingid, e := retrieveID(cs,
"service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
- // Create a new parameter struct
- p :=
cs.VirtualMachine.NewChangeServiceForVirtualMachineParams(d.Id(),
serviceofferingid)
+ // Create a new parameter struct for scaling
+ p :=
cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), serviceofferingid)
- // Change the service offering
- _, err =
cs.VirtualMachine.ChangeServiceForVirtualMachine(p)
+ // Scale the VM to the new service offering
+ _, err = cs.VirtualMachine.ScaleVirtualMachine(p)
Review Comment:
Current acceptance coverage for instance resize appears to assert only that
`service_offering` / `Serviceofferingname` changes (e.g.,
`resource_cloudstack_instance_test.go:testAccCheckCloudStackInstanceRenamedAndResized`),
which would have passed with the previous `ChangeServiceForVirtualMachine`
call as well. To prevent regressions, consider extending the update test to
assert the VM’s actual CPU/memory values (or another observable effect) after
the service offering update so it specifically validates `ScaleVirtualMachine`
behavior.
--
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]
