Copilot commented on code in PR #204:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/204#discussion_r2300249092
##########
cloudstack/resource_cloudstack_port_forward.go:
##########
@@ -288,6 +306,21 @@ func resourceCloudStackPortForwardRead(d
*schema.ResourceData, meta interface{})
forward["protocol"] = f.Protocol
forward["private_port"] = privPort
forward["public_port"] = pubPort
+ // Only set end ports if they differ from start ports
(indicating a range)
+ if f.Privateendport != "" && f.Privateendport !=
f.Privateport {
+ privEndPort, err :=
strconv.Atoi(f.Privateendport)
+ if err != nil {
+ return err
+ }
+ forward["private_end_port"] = privEndPort
+ }
+ if f.Publicendport != "" && f.Publicendport !=
f.Publicport {
+ pubEndPort, err := strconv.Atoi(f.Publicendport)
+ if err != nil {
+ return err
+ }
+ forward["public_end_port"] = pubEndPort
Review Comment:
[nitpick] The logic for handling end ports is duplicated for both private
and public ports. Consider extracting this into a helper function to reduce
code duplication and improve maintainability.
```suggestion
if err := setEndPort(forward, "private_end_port",
f.Privateendport, f.Privateport); err != nil {
return err
}
if err := setEndPort(forward, "public_end_port",
f.Publicendport, f.Publicport); err != nil {
return err
```
##########
cloudstack/resource_cloudstack_port_forward.go:
##########
@@ -288,6 +306,21 @@ func resourceCloudStackPortForwardRead(d
*schema.ResourceData, meta interface{})
forward["protocol"] = f.Protocol
forward["private_port"] = privPort
forward["public_port"] = pubPort
+ // Only set end ports if they differ from start ports
(indicating a range)
+ if f.Privateendport != "" && f.Privateendport !=
f.Privateport {
+ privEndPort, err :=
strconv.Atoi(f.Privateendport)
+ if err != nil {
+ return err
+ }
+ forward["private_end_port"] = privEndPort
+ }
+ if f.Publicendport != "" && f.Publicendport !=
f.Publicport {
+ pubEndPort, err := strconv.Atoi(f.Publicendport)
+ if err != nil {
+ return err
+ }
+ forward["public_end_port"] = pubEndPort
Review Comment:
[nitpick] The logic for handling end ports is duplicated for both private
and public ports. Consider extracting this into a helper function to reduce
code duplication and improve maintainability.
```suggestion
if err := setEndPort(forward, "private_end_port",
f.Privateendport, f.Privateport); err != nil {
return err
}
if err := setEndPort(forward, "public_end_port",
f.Publicendport, f.Publicport); err != nil {
return err
```
##########
cloudstack/resource_cloudstack_port_forward.go:
##########
@@ -175,6 +187,12 @@ func createPortForward(d *schema.ResourceData, meta
interface{}, forward map[str
// Create a new parameter struct
p := cs.Firewall.NewCreatePortForwardingRuleParams(d.Id(),
forward["private_port"].(int),
forward["protocol"].(string), forward["public_port"].(int),
vm.Id)
+ if val, ok := forward["private_end_port"]; ok && val != nil &&
val.(int) != 0 {
+ p.SetPrivateendport(val.(int))
+ }
+ if val, ok := forward["public_end_port"]; ok && val != nil && val.(int)
!= 0 {
+ p.SetPublicendport(val.(int))
+ }
Review Comment:
The condition `val.(int) != 0` will prevent setting port 0 as an end port,
which might be a valid use case. Consider removing this check or validating
that the end port is greater than the start port instead.
--
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: dev-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
