Copilot commented on code in PR #285:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/285#discussion_r3240173298
##########
cloudstack/resource_cloudstack_domain.go:
##########
@@ -41,6 +41,8 @@ func resourceCloudStackDomain() *schema.Resource {
"domain_id": {
Type: schema.TypeString,
Optional: true,
Review Comment:
`domain_id` is now marked as `Optional` + `Computed` + `ForceNew`, but
Read() always sets it to the domain's own ID. If a caller still sets
`domain_id` in config (allowed by `Optional: true`), it will be overwritten on
refresh and then force perpetual recreation. Consider making `domain_id`
computed-only (remove `Optional`) if it's meant to be an output attribute, or
stop setting it in Read() if it's meant as an input.
##########
cloudstack/resource_cloudstack_account.go:
##########
@@ -110,9 +113,107 @@ func resourceCloudStackAccountCreate(d
*schema.ResourceData, meta interface{}) e
return resourceCloudStackAccountRead(d, meta)
}
-func resourceCloudStackAccountRead(d *schema.ResourceData, meta interface{})
error { return nil }
+func resourceCloudStackAccountRead(d *schema.ResourceData, meta interface{})
error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ log.Printf("[DEBUG] Reading Account %s", d.Id())
Review Comment:
PR description mentions "Added test configurations under tests/" but the
repository currently has no `tests/` directory (only Go *_test.go files under
`cloudstack/`). Either update the PR description to reflect the actual test
additions/changes, or include the intended test configs if they are part of
this PR.
##########
cloudstack/resource_cloudstack_account.go:
##########
@@ -110,9 +113,107 @@ func resourceCloudStackAccountCreate(d
*schema.ResourceData, meta interface{}) e
return resourceCloudStackAccountRead(d, meta)
}
-func resourceCloudStackAccountRead(d *schema.ResourceData, meta interface{})
error { return nil }
+func resourceCloudStackAccountRead(d *schema.ResourceData, meta interface{})
error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ log.Printf("[DEBUG] Reading Account %s", d.Id())
-func resourceCloudStackAccountUpdate(d *schema.ResourceData, meta interface{})
error { return nil }
+ p := cs.Account.NewListAccountsParams()
+ p.SetId(d.Id())
+
+ accounts, err := cs.Account.ListAccounts(p)
+ if err != nil {
+ return fmt.Errorf("Error retrieving Account %s: %s", d.Id(),
err)
+ }
+
+ if accounts.Count == 0 {
+ log.Printf("[DEBUG] Account %s does no longer exist", d.Id())
+ d.SetId("")
+ return nil
+ }
+
+ account := accounts.Accounts[0]
+
+ d.Set("account_type", account.Accounttype)
+ d.Set("role_id", account.Roleid)
+ d.Set("account", account.Name)
+ d.Set("domain_id", account.Domainid)
+
+ if len(account.User) > 0 {
+ user := account.User[0]
+ d.Set("email", user.Email)
+ d.Set("first_name", user.Firstname)
+ d.Set("last_name", user.Lastname)
+ d.Set("username", user.Username)
+ }
+
+ log.Printf("[DEBUG] Account %s successfully read", d.Id())
+ return nil
+}
+
+func resourceCloudStackAccountUpdate(d *schema.ResourceData, meta interface{})
error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ log.Printf("[DEBUG] Updating Account %s", d.Id())
+
+ // Handle account-level changes
+ if d.HasChange("role_id") || d.HasChange("account") ||
d.HasChange("domain_id") {
+ p := cs.Account.NewUpdateAccountParams()
+ p.SetId(d.Id())
+
+ if d.HasChange("role_id") {
+ p.SetRoleid(d.Get("role_id").(string))
+ }
+ if d.HasChange("account") {
+ p.SetNewname(d.Get("account").(string))
+ }
+ if d.HasChange("domain_id") {
+ p.SetDomainid(d.Get("domain_id").(string))
+ }
+
+ _, err := cs.Account.UpdateAccount(p)
+ if err != nil {
+ return fmt.Errorf("Error updating Account %s: %s",
d.Id(), err)
+ }
+ }
+
+ // Handle user-level changes via updateUser API
+ if d.HasChange("email") || d.HasChange("first_name") ||
d.HasChange("last_name") || d.HasChange("password") {
+ lp := cs.Account.NewListAccountsParams()
+ lp.SetId(d.Id())
+ accounts, err := cs.Account.ListAccounts(lp)
+ if err != nil {
+ return fmt.Errorf("Error retrieving Account %s for user
update: %s", d.Id(), err)
+ }
+ if accounts.Count == 0 || len(accounts.Accounts[0].User) == 0 {
+ return fmt.Errorf("Account %s has no users to update",
d.Id())
+ }
+
+ userID := accounts.Accounts[0].User[0].Id
Review Comment:
Update() uses `accounts.Accounts[0].User[0].Id` for user-level updates. For
accounts with multiple users, this can update the wrong user (e.g., reset a
different user's password/email). Consider locating the correct user by
matching `username` from state/config, or storing the created user's ID in
state and using that for updates.
##########
cloudstack/resource_cloudstack_account.go:
##########
@@ -110,9 +113,107 @@ func resourceCloudStackAccountCreate(d
*schema.ResourceData, meta interface{}) e
return resourceCloudStackAccountRead(d, meta)
}
-func resourceCloudStackAccountRead(d *schema.ResourceData, meta interface{})
error { return nil }
+func resourceCloudStackAccountRead(d *schema.ResourceData, meta interface{})
error {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ log.Printf("[DEBUG] Reading Account %s", d.Id())
-func resourceCloudStackAccountUpdate(d *schema.ResourceData, meta interface{})
error { return nil }
+ p := cs.Account.NewListAccountsParams()
+ p.SetId(d.Id())
+
+ accounts, err := cs.Account.ListAccounts(p)
+ if err != nil {
+ return fmt.Errorf("Error retrieving Account %s: %s", d.Id(),
err)
+ }
+
+ if accounts.Count == 0 {
+ log.Printf("[DEBUG] Account %s does no longer exist", d.Id())
+ d.SetId("")
+ return nil
+ }
+
+ account := accounts.Accounts[0]
+
+ d.Set("account_type", account.Accounttype)
+ d.Set("role_id", account.Roleid)
+ d.Set("account", account.Name)
+ d.Set("domain_id", account.Domainid)
+
+ if len(account.User) > 0 {
+ user := account.User[0]
+ d.Set("email", user.Email)
+ d.Set("first_name", user.Firstname)
+ d.Set("last_name", user.Lastname)
+ d.Set("username", user.Username)
+ }
Review Comment:
Read() picks the first user in `account.User` (`account.User[0]`) to
populate email/name/username. If an account has multiple users, this can make
state non-deterministic and may reflect a different user than the one
originally created/managed by this resource. Consider selecting the user that
matches the configured/state `username` (or persist a `user_id` in state)
instead of always taking index 0.
--
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]
