Copilot commented on code in PR #289:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/289#discussion_r3026421132
##########
cloudstack/resource_cloudstack_network.go:
##########
@@ -190,23 +190,25 @@ func resourceCloudStackNetworkCreate(d
*schema.ResourceData, meta interface{}) e
return err
}
- m, err := parseCIDR(d, no.Specifyipranges)
- if err != nil {
- return err
- }
+ if _, ok := d.GetOk("cidr"); ok {
+ m, err := parseCIDR(d, no.Specifyipranges)
+ if err != nil {
+ return err
+ }
Review Comment:
Now that `cidr` is optional, users can set `gateway`/`startip`/`endip`
without setting `cidr`. In that case this block is skipped and those arguments
are silently ignored, which can lead to confusing behavior and perpetual diffs
(e.g., config sets gateway but API/state stays empty). Consider validating that
`gateway`, `startip`, and `endip` may only be set when `cidr` is also set
(e.g., schema `RequiredWith`/`ConflictsWith` or an explicit check in Create
returning a clear error).
##########
cloudstack/resource_cloudstack_network.go:
##########
@@ -190,23 +190,25 @@ func resourceCloudStackNetworkCreate(d
*schema.ResourceData, meta interface{}) e
return err
}
- m, err := parseCIDR(d, no.Specifyipranges)
- if err != nil {
- return err
- }
+ if _, ok := d.GetOk("cidr"); ok {
+ m, err := parseCIDR(d, no.Specifyipranges)
+ if err != nil {
Review Comment:
With `cidr` optional, `GetNetworkOfferingByID` is only needed when `cidr` is
present (to read `Specifyipranges` for `parseCIDR`). To avoid an extra API call
on L2/no-cidr networks, consider moving the network-offering lookup inside the
`cidr` conditional block.
--
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]
