Copilot commented on code in PR #262:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/262#discussion_r2911014084
##########
cloudstack/resources.go:
##########
@@ -162,6 +162,19 @@ func setProjectid(p cloudstack.ProjectIDSetter, cs
*cloudstack.CloudStackClient,
return nil
}
+// If there is a domain supplied, we retrieve and set the domain id
+func setDomainid(p cloudstack.DomainIDSetter, cs *cloudstack.CloudStackClient,
d *schema.ResourceData) error {
+ if domain, ok := d.GetOk("domain"); ok {
+ domainid, e := retrieveID(cs, "domain", domain.(string))
+ if e != nil {
+ return e.Error()
+ }
+ p.SetDomainid(domainid)
+ }
+
+ return nil
+}
Review Comment:
`setDomainid` is introduced but not used anywhere in the current codebase
(search only finds its definition). If this helper is intended for upcoming
changes, consider adding the call sites in this PR; otherwise, it’s dead code
that increases maintenance surface and can be removed until needed.
##########
cloudstack/resource_cloudstack_security_group.go:
##########
@@ -97,7 +140,7 @@ func resourceCloudStackSecurityGroupRead(d
*schema.ResourceData, meta interface{
// Get the security group details
sg, count, err := cs.SecurityGroup.GetSecurityGroupByID(
d.Id(),
- cloudstack.WithProject(d.Get("project").(string)),
+ cloudstack.WithProject(d.Get("projectid").(string)),
)
Review Comment:
Import uses importStatePassthrough, which populates the state attribute
`project`, but this resource now reads project context from `projectid`
(WithProject(d.Get("projectid"))). Importing with a project prefix (e.g.,
`my-project/<id>`) will therefore lose the project scope and may fail to
read/delete the SG. Consider switching this resource to a dedicated importer
that sets `projectid` from the prefix (and optionally also supports legacy
`project` for backward compatibility).
##########
cloudstack/resource_cloudstack_security_group.go:
##########
@@ -66,6 +79,18 @@ func resourceCloudStackSecurityGroupCreate(d
*schema.ResourceData, meta interfac
name := d.Get("name").(string)
+ // Validate that account is used with domainid
+ if account, ok := d.GetOk("account"); ok {
+ if _, domainOk := d.GetOk("domainid"); !domainOk {
+ return fmt.Errorf("account parameter requires domainid
to be set")
+ }
+ // Account and projectid are mutually exclusive
+ if _, projectOk := d.GetOk("projectid"); projectOk {
+ return fmt.Errorf("account and projectid parameters are
mutually exclusive")
+ }
+ log.Printf("[DEBUG] Creating security group %s for account %s",
name, account)
+ }
Review Comment:
`account` from `d.GetOk("account")` is an `interface{}`; logging it with
`%s` will produce `%!s(interface {}=...)` in logs. Cast to string (or use `%v`)
before logging.
--
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]
