Copilot commented on code in PR #289:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/289#discussion_r3153896992
##########
cloudstack/resource_cloudstack_network.go:
##########
@@ -74,7 +74,8 @@ func resourceCloudStackNetwork() *schema.Resource {
"cidr": {
Type: schema.TypeString,
- Required: true,
+ Optional: true,
+ Computed: true,
ForceNew: true,
},
Review Comment:
The PR title/description says the goal is to allow CIDR-less *L2* network
creation, but this schema change makes `cidr` optional/computed for **all**
`cloudstack_network` instances. If only L2 should allow omission, consider
keeping `cidr` required in the schema and enforcing/relaxing the requirement in
`Create` based on the network offering’s guest IP type (e.g., `Guestiptype ==
"L2"`), or update the PR title/description to reflect the broader behavior
change.
##########
cloudstack/resource_cloudstack_network.go:
##########
@@ -190,23 +191,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
+ }
- // Set the needed IP config
- p.SetGateway(m["gateway"])
- p.SetNetmask(m["netmask"])
+ // Set the needed IP config
+ p.SetGateway(m["gateway"])
+ p.SetNetmask(m["netmask"])
- // Only set the start IP if we have one
- if startip, ok := m["startip"]; ok {
- p.SetStartip(startip)
- }
+ // Only set the start IP if we have one
+ if startip, ok := m["startip"]; ok {
+ p.SetStartip(startip)
+ }
- // Only set the end IP if we have one
- if endip, ok := m["endip"]; ok {
- p.SetEndip(endip)
+ // Only set the end IP if we have one
+ if endip, ok := m["endip"]; ok {
+ p.SetEndip(endip)
+ }
}
Review Comment:
Now that `cidr` is optional, users can provide `gateway`/`startip`/`endip`
without providing `cidr`, but this block will silently ignore those values (and
`netmask` can’t be derived). Add explicit validation (e.g., return an error)
when any of these IP config fields are set but `cidr` is not, and/or require
`cidr` for non-L2 offerings to avoid surprising applies and downstream
CloudStack API errors.
##########
cloudstack/resource_cloudstack_network_test.go:
##########
@@ -377,3 +377,59 @@ resource "cloudstack_network" "foo" {
acl_id = cloudstack_network_acl.bar.id
zone = cloudstack_vpc.foo.zone
}`
+
+func TestAccCloudStackNetwork_l2NoCidr(t *testing.T) {
+ var network cloudstack.Network
+
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackNetworkDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccCloudStackNetwork_l2NoCidr,
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckCloudStackNetworkExists(
+ "cloudstack_network.l2",
&network),
+ ),
+ },
+ },
+ })
+}
+
+const testAccCloudStackNetwork_l2NoCidr = `
+resource "cloudstack_network" "l2" {
+ name = "terraform-l2-network"
+ display_text = "terraform-l2-network"
+ network_offering = "DefaultL2NetworkOffering"
+ zone = "Sandbox-simulator"
+}`
+
+func TestAccCloudStackNetwork_isolatedNoCidr(t *testing.T) {
+ var network cloudstack.Network
+
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackNetworkDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccCloudStackNetwork_isolatedNoCidr,
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckCloudStackNetworkExists(
+
"cloudstack_network.isolated_no_cidr", &network),
+ resource.TestCheckResourceAttrSet(
+
"cloudstack_network.isolated_no_cidr", "cidr"),
+ ),
Review Comment:
This PR is described as enabling CIDR-less **L2** network creation, but this
acceptance test covers an *isolated* offering without `cidr` and asserts that
`cidr` becomes set. If the intent is L2-only, this test (and the broader
behavior it implies) looks out of scope; otherwise, consider clarifying the PR
title/description and ensuring the provider validates/handles non-L2 CIDR
omission consistently.
--
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]
