Copilot commented on code in PR #268:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/268#discussion_r3782345174
##########
cloudstack/resource_cloudstack_instance_test.go:
##########
@@ -295,6 +295,45 @@ func TestAccCloudStackInstance_userData(t *testing.T) {
})
}
+func TestAccCloudStackInstance_deleteProtection(t *testing.T) {
+ var instance cloudstack.VirtualMachine
+
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackDiskDestroy,
Review Comment:
`TestAccCloudStackInstance_deleteProtection` sets `CheckDestroy` to
`testAccCheckCloudStackDiskDestroy`, which is the disk destroy-check helper.
This will validate the wrong resource type (and likely fail or provide
misleading results). Use the instance destroy-check helper for instance tests
(consistent with other instance acceptance tests).
##########
cloudstack/resource_cloudstack_disk.go:
##########
@@ -92,13 +92,19 @@ func resourceCloudStackDisk() *schema.Resource {
ForceNew: true,
},
- "tags": tagsSchema(),
-
"reattach_on_change": {
Type: schema.TypeBool,
Optional: true,
Default: false,
},
+
+ "delete_protection": {
+ Type: schema.TypeBool,
+ Optional: true,
+ Computed: true,
Review Comment:
`delete_protection` is `Computed: true`, but `resourceCloudStackDiskRead`
(in the provided context) does not set this field from the retrieved volume.
This prevents correct state refresh (especially for import or external toggles)
and is inconsistent with using `Computed`. Set `delete_protection` during Read
based on the CloudStack volume response, or drop `Computed` if it’s meant to be
purely config-driven.
##########
cloudstack/resource_cloudstack_instance.go:
##########
@@ -249,6 +249,12 @@ func resourceCloudStackInstance() *schema.Resource {
Optional: true,
},
+ "delete_protection": {
+ Type: schema.TypeBool,
+ Optional: true,
+ Computed: true,
Review Comment:
`delete_protection` is marked `Computed: true`, but the current
`resourceCloudStackInstanceRead` implementation (in the provided context) does
not set `delete_protection` from the API response. With `Computed` set,
Terraform expects Read to populate it; otherwise state can be stale (imports
won’t get the value, and out-of-band changes won’t be reflected). Update the
Read function to `d.Set(\"delete_protection\", <value-from-CloudStack>)`, or
remove `Computed` if you don’t intend to track remote state.
--
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]
