Copilot commented on code in PR #218:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/218#discussion_r2371922678
##########
website/docs/r/egress_firewall.html.markdown:
##########
@@ -55,7 +105,7 @@ The `rule` block supports:
the protocol is ICMP.
* `ports` - (Optional) List of ports and/or port ranges to allow. This can only
- be specified if the protocol is TCP or UDP.
+ be specified if the protocol is TCP or UDP. For TCP/UDP, omitting `ports`
creates an all-ports rule. CloudStack may represent this as empty start/end,
`0/0`, or `1/65535`; the provider handles all.
Review Comment:
The sentence structure is unclear and contains a grammatical error. Consider
revising to: 'be specified if the protocol is TCP or UDP. For TCP/UDP
protocols, omitting `ports` creates an all-ports rule. CloudStack may represent
this as empty start/end ports, `0/0`, or `1/65535`; the provider handles all
formats.'
```suggestion
be specified if the protocol is TCP or UDP. For TCP/UDP protocols,
omitting `ports` creates an all-ports rule. CloudStack may represent this as
empty start/end ports, `0/0`, or `1/65535`; the provider handles all formats.
```
##########
cloudstack/resource_cloudstack_egress_firewall.go:
##########
@@ -31,6 +32,81 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
)
+// treats 'all ports' for tcp/udp across CS versions returning 0/0, -1/-1, or
1/65535
Review Comment:
The comment has grammatical issues. Consider revising to: '//
isAllPortsTCPUDP determines if a rule represents all ports for TCP/UDP
protocols across CloudStack versions that may return 0/0, -1/-1, or 1/65535'
```suggestion
// isAllPortsTCPUDP determines if a rule represents all ports for TCP/UDP
protocols across CloudStack versions that may return 0/0, -1/-1, or 1/65535
```
##########
cloudstack/resource_cloudstack_egress_firewall.go:
##########
@@ -606,10 +757,9 @@ func verifyEgressFirewallRuleParams(d
*schema.ResourceData, rule map[string]inte
"%q is not a valid port value.
Valid options are '80' or '80-90'", port.(string))
}
}
- } else {
- return fmt.Errorf(
- "Parameter ports is a required parameter when
*not* using protocol 'icmp'")
}
+ // Note: ports parameter is optional for TCP/UDP protocols
+ // When omitted, the rule will encompass all ports
Review Comment:
[nitpick] These comments are placed in an odd location within the validation
logic. Consider moving them to the function-level documentation or closer to
where the actual logic handles the omitted ports case for better code
organization.
--
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]
