Copilot commented on code in PR #284:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/284#discussion_r2910366844
##########
cloudstack/resource_cloudstack_static_route.go:
##########
@@ -58,11 +75,20 @@ func resourceCloudStackStaticRouteCreate(d
*schema.ResourceData, meta interface{
d.Get("cidr").(string),
)
+ // Set either gateway_id or nexthop+vpc_id (they are mutually exclusive)
if v, ok := d.GetOk("gateway_id"); ok {
p.SetGatewayid(v.(string))
}
- // Create the new private gateway
+ if v, ok := d.GetOk("nexthop"); ok {
+ p.SetNexthop(v.(string))
+ }
+
+ if v, ok := d.GetOk("vpc_id"); ok {
+ p.SetVpcid(v.(string))
+ }
Review Comment:
Because `gateway_id` is no longer required, Create can be invoked with
neither `gateway_id` nor `nexthop` set (or with only one of the
`nexthop`/`vpc_id` pair if schema validation changes later). Consider adding an
explicit parameter verification at the start of Create (similar to
`verifyIPAddressParams`) and return a clear error when neither routing mode is
selected or when `nexthop`/`vpc_id` are incomplete.
##########
cloudstack/resource_cloudstack_static_route_test.go:
##########
@@ -42,6 +42,32 @@ func TestAccCloudStackStaticRoute_basic(t *testing.T) {
testAccCheckCloudStackStaticRouteExists(
"cloudstack_static_route.foo",
&staticroute),
testAccCheckCloudStackStaticRouteAttributes(&staticroute),
+ resource.TestCheckResourceAttr(
+ "cloudstack_static_route.foo",
"cidr", "172.16.0.0/16"),
+ ),
+ },
+ },
+ })
+}
+
+func TestAccCloudStackStaticRoute_nexthop(t *testing.T) {
+ var staticroute cloudstack.StaticRoute
+
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheckStaticRouteNexthop(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackStaticRouteDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccCloudStackStaticRoute_nexthop,
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckCloudStackStaticRouteExists(
+ "cloudstack_static_route.bar",
&staticroute),
+
testAccCheckCloudStackStaticRouteNexthopAttributes(&staticroute),
+ resource.TestCheckResourceAttr(
+ "cloudstack_static_route.bar",
"cidr", "192.168.0.0/16"),
+ resource.TestCheckResourceAttr(
+ "cloudstack_static_route.bar",
"nexthop", "10.1.1.1"),
),
Review Comment:
The new nexthop acceptance test asserts `cidr` and `nexthop`, but it doesn't
assert that `vpc_id` is actually persisted in state and matches the created
VPC. Adding a `TestCheckResourceAttrPair` between
`cloudstack_static_route.bar.vpc_id` and `cloudstack_vpc.bar.id` would better
cover the new argument and catch Read/state issues.
##########
cloudstack/resource_cloudstack_static_route.go:
##########
@@ -42,9 +42,26 @@ func resourceCloudStackStaticRoute() *schema.Resource {
},
"gateway_id": {
- Type: schema.TypeString,
- Required: true,
- ForceNew: true,
+ Type: schema.TypeString,
+ Optional: true,
+ ForceNew: true,
+ ConflictsWith: []string{"nexthop", "vpc_id"},
+ },
+
+ "nexthop": {
+ Type: schema.TypeString,
+ Optional: true,
+ ForceNew: true,
+ ConflictsWith: []string{"gateway_id"},
+ RequiredWith: []string{"vpc_id"},
+ },
+
+ "vpc_id": {
+ Type: schema.TypeString,
+ Optional: true,
+ ForceNew: true,
+ ConflictsWith: []string{"gateway_id"},
+ RequiredWith: []string{"nexthop"},
},
Review Comment:
The schema now makes `gateway_id`, `nexthop`, and `vpc_id` all optional, but
there is no enforcement that the user must set either `gateway_id` or `nexthop`
(with `vpc_id`). This allows configs like `{ cidr = ... }` to pass validation
and then fail at the API layer with a less actionable error. Add schema-level
enforcement (e.g., `ExactlyOneOf` on `gateway_id`/`nexthop` or an explicit
validation via `CustomizeDiff`) so Terraform rejects invalid combinations
before Create is called.
##########
website/docs/r/static_route.html.markdown:
##########
@@ -12,22 +12,43 @@ Creates a static route for the given private gateway or VPC.
## Example Usage
+Using a private gateway:
+
```hcl
resource "cloudstack_static_route" "default" {
cidr = "10.0.0.0/16"
gateway_id = "76f607e3-e8dc-4971-8831-b2a2b0cc4cb4"
}
```
+Using a nexthop IP address:
+
+```hcl
+resource "cloudstack_static_route" "with_nexthop" {
+ cidr = "10.0.0.0/16"
+ nexthop = "192.168.1.1"
+ vpc_id = "76f607e3-e8dc-4971-8831-b2a2b0cc4cb4"
+}
+```
+
## Argument Reference
The following arguments are supported:
* `cidr` - (Required) The CIDR for the static route. Changing this forces
a new resource to be created.
-* `gateway_id` - (Required) The ID of the Private gateway. Changing this forces
- a new resource to be created.
+* `gateway_id` - (Optional) The ID of the Private gateway. Changing this forces
+ a new resource to be created. Conflicts with `nexthop` and `vpc_id`.
+
+* `nexthop` - (Optional) The IP address of the nexthop for the static route.
+ Changing this forces a new resource to be created. Conflicts with
`gateway_id`.
+ Must be used together with `vpc_id`.
+
+* `vpc_id` - (Optional) The ID of the VPC. Required when using `nexthop`.
+ Changing this forces a new resource to be created. Conflicts with
`gateway_id`.
+
+**Note:** Either `gateway_id` or (`nexthop` + `vpc_id`) must be specified.
Review Comment:
The documentation describes `nexthop` usage but does not mention the
CloudStack version requirement (4.22+) that is called out in the PR description
and enforced by the acceptance-test precheck. Add a short note near the
`nexthop` argument (or the existing **Note**) indicating the minimum CloudStack
version needed for `nexthop` support.
--
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]
