Copilot commented on code in PR #285:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/285#discussion_r2910122910
##########
cloudstack/resource_cloudstack_disk_offering.go:
##########
@@ -72,8 +73,59 @@ func resourceCloudStackDiskOfferingCreate(d
*schema.ResourceData, meta interface
return resourceCloudStackDiskOfferingRead(d, meta)
}
-func resourceCloudStackDiskOfferingRead(d *schema.ResourceData, meta
interface{}) error { return nil }
+func resourceCloudStackDiskOfferingRead(d *schema.ResourceData, meta
interface{}) error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ log.Printf("[DEBUG] Retrieving disk offering %s",
d.Get("name").(string))
+
+ offering, count, err := cs.DiskOffering.GetDiskOfferingByID(d.Id())
+ if err != nil {
+ if count == 0 {
+ log.Printf("[DEBUG] Disk offering %s does no longer
exist", d.Get("name").(string))
+ d.SetId("")
+ return nil
+ }
+ return fmt.Errorf("Error retrieving disk offering %s: %s",
d.Id(), err)
+ }
+
+ d.Set("name", offering.Name)
+ d.Set("display_text", offering.Displaytext)
+ d.Set("disk_size", offering.Disksize)
+
+ return nil
+}
+
+func resourceCloudStackDiskOfferingUpdate(d *schema.ResourceData, meta
interface{}) error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ p := cs.DiskOffering.NewUpdateDiskOfferingParams(d.Id())
+
+ if d.HasChange("name") {
+ p.SetName(d.Get("name").(string))
+ }
+ if d.HasChange("display_text") {
+ p.SetDisplaytext(d.Get("display_text").(string))
+ }
Review Comment:
`disk_size` is required and not `ForceNew`, but Update() never applies
changes to it. If a user changes `disk_size`, Terraform will call Update but
the remote value won't change, leaving the resource permanently drifted. Either
implement updating the size (if CloudStack supports it) or mark `disk_size` as
`ForceNew` to force recreation.
##########
tests/comprehensive/versions.tf:
##########
@@ -0,0 +1,9 @@
+terraform {
+ required_providers {
+ cloudstack = {
+ source = "local/cloudstack/cloudstack"
+ version = "0.5.0"
+ }
Review Comment:
The provider source address `local/cloudstack/cloudstack` and pinned version
`0.5.0` are likely to make `terraform init` fail for most users (Terraform will
try to download from host `local`, and v0.5.0 may be unavailable). Prefer
`source = "cloudstack/cloudstack"` and a flexible version constraint (or
document the required dev_override setup explicitly).
##########
cloudstack/resource_cloudstack_user.go:
##########
@@ -88,6 +88,28 @@ func resourceCloudStackUserCreate(d *schema.ResourceData,
meta interface{}) erro
}
func resourceCloudStackUserUpdate(d *schema.ResourceData, meta interface{})
error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ p := cs.User.NewUpdateUserParams(d.Id())
+
+ if d.HasChange("email") {
+ p.SetEmail(d.Get("email").(string))
+ }
+ if d.HasChange("first_name") {
+ p.SetFirstname(d.Get("first_name").(string))
+ }
+ if d.HasChange("last_name") {
+ p.SetLastname(d.Get("last_name").(string))
+ }
+ if d.HasChange("password") {
+ p.SetPassword(d.Get("password").(string))
+ }
+
+ _, err := cs.User.UpdateUser(p)
+ if err != nil {
+ return err
+ }
Review Comment:
Returning the raw error from `cs.User.UpdateUser` loses resource context
(ID/username) and makes debugging harder. Wrap the error with a message that
includes the user ID (and possibly username) similar to other resources in this
provider.
##########
tests/comprehensive/main.tf:
##########
@@ -0,0 +1,330 @@
+# Test Infrastructure for CloudStack Terraform Provider Verification
+# This tests all claimed provider capabilities and limitations
+
+provider "cloudstack" {
+ # Using environment variables: CLOUDSTACK_API_URL, CLOUDSTACK_API_KEY,
CLOUDSTACK_SECRET_KEY
+}
+
+# Generate unique test identifier
+locals {
+ test_prefix = "tf-test"
+}
Review Comment:
The provider block says it uses environment variables, but the test suite
also defines `api_url/api_key/secret_key` variables and a
`terraform.tfvars.example` for them, and `test_prefix` is defined as a variable
but then hard-coded in `locals`. This mismatch is confusing for running the
tests; consider either wiring the provider to the variables (and using
`var.test_prefix`) or removing the unused variables/tfvars example inputs.
##########
cloudstack/resource_cloudstack_user.go:
##########
@@ -88,6 +88,28 @@ func resourceCloudStackUserCreate(d *schema.ResourceData,
meta interface{}) erro
}
func resourceCloudStackUserUpdate(d *schema.ResourceData, meta interface{})
error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ p := cs.User.NewUpdateUserParams(d.Id())
+
+ if d.HasChange("email") {
+ p.SetEmail(d.Get("email").(string))
+ }
+ if d.HasChange("first_name") {
+ p.SetFirstname(d.Get("first_name").(string))
+ }
+ if d.HasChange("last_name") {
+ p.SetLastname(d.Get("last_name").(string))
+ }
+ if d.HasChange("password") {
+ p.SetPassword(d.Get("password").(string))
+ }
Review Comment:
Update() only handles email/first_name/last_name/password, but the schema
also allows changes to `account` and `username`. If either of those fields
changes, Terraform will plan an update but this implementation will ignore the
change, causing persistent drift. Either support updating those fields via the
API or mark them `ForceNew` so Terraform recreates instead of attempting Update.
##########
tests/comprehensive/main.tf:
##########
@@ -0,0 +1,330 @@
+# Test Infrastructure for CloudStack Terraform Provider Verification
+# This tests all claimed provider capabilities and limitations
+
+provider "cloudstack" {
+ # Using environment variables: CLOUDSTACK_API_URL, CLOUDSTACK_API_KEY,
CLOUDSTACK_SECRET_KEY
+}
+
+# Generate unique test identifier
+locals {
+ test_prefix = "tf-test"
+}
+
+# ============================================
+# TEST 1: SSH Keypair (Tests: missing Update function)
+# ============================================
+
+resource "cloudstack_ssh_keypair" "test" {
+ name = "${local.test_prefix}-keypair"
+ public_key = file("~/.ssh/id_rsa.pub")
+}
Review Comment:
`file("~/.ssh/id_rsa.pub")` will not expand `~` in Terraform, so this test
config will fail unless run from a directory where that literal path exists.
Use `file(pathexpand("~/.ssh/id_rsa.pub"))` or make the public key path a
variable.
##########
cloudstack/resource_cloudstack_domain.go:
##########
@@ -89,9 +89,62 @@ func resourceCloudStackDomainCreate(d *schema.ResourceData,
meta interface{}) er
return resourceCloudStackDomainRead(d, meta)
}
-func resourceCloudStackDomainRead(d *schema.ResourceData, meta interface{})
error { return nil }
+func resourceCloudStackDomainRead(d *schema.ResourceData, meta interface{})
error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ log.Printf("[DEBUG] Reading Domain %s", d.Id())
+
+ p := cs.Domain.NewListDomainsParams()
+ p.SetId(d.Id())
+
+ domains, err := cs.Domain.ListDomains(p)
+ if err != nil {
+ return fmt.Errorf("Error reading Domain %s: %s", d.Id(), err)
+ }
+
+ if domains.Count == 0 {
+ log.Printf("[DEBUG] Domain %s does no longer exist", d.Id())
+ d.SetId("")
+ return nil
+ }
+
+ domain := domains.Domains[0]
+ log.Printf("[DEBUG] Domain %s found: %s", d.Id(), domain.Name)
+
+ d.Set("name", domain.Name)
+ d.Set("domain_id", domain.Id)
+ d.Set("network_domain", domain.Networkdomain)
Review Comment:
In Read(), setting `domain_id` to `domain.Id` will overwrite any configured
`domain_id` value (it is `Optional` in the schema) and can cause perpetual
diffs / unexpected behavior. Either make `domain_id` a computed-only attribute
(matching the data source) or stop setting it here and instead keep it purely
as an input field (e.g., only used during Create).
##########
cloudstack/resource_cloudstack_account.go:
##########
@@ -37,26 +37,32 @@ func resourceCloudStackAccount() *schema.Resource {
"email": {
Type: schema.TypeString,
Required: true,
+ ForceNew: true,
},
"first_name": {
Type: schema.TypeString,
Required: true,
+ ForceNew: true,
},
"last_name": {
Type: schema.TypeString,
Required: true,
+ ForceNew: true,
},
"password": {
Type: schema.TypeString,
Required: true,
+ ForceNew: true,
},
Review Comment:
`password` is a required credential but is not marked `Sensitive: true` in
the schema. This can leak secrets into plan output and state diffs. Mark this
attribute as sensitive (consistent with other resources like
`cloudstack_host.password`).
--
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]
