Copilot commented on code in PR #204:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/204#discussion_r2297917977
##########
cloudstack/resource_cloudstack_port_forward.go:
##########
@@ -279,15 +295,31 @@ func resourceCloudStackPortForwardRead(d
*schema.ResourceData, meta interface{})
return err
}
+ privEndPort, err := strconv.Atoi(f.Privateendport)
+ if err != nil {
+ return err
+ }
+
pubPort, err := strconv.Atoi(f.Publicport)
if err != nil {
return err
}
+ pubEndPort, err := strconv.Atoi(f.Publicendport)
+ if err != nil {
+ return err
Review Comment:
The code attempts to parse `f.Privateendport` and `f.Publicendport` without
checking if they are empty strings first. If these fields are empty,
`strconv.Atoi` will return an error. You should check if the strings are
non-empty before parsing, similar to how it's done in the conditional logic
below.
```suggestion
// Only parse Publicendport if it's non-empty and not
equal to Publicport
var pubEndPort int
if f.Publicendport != "" && f.Publicendport !=
f.Publicport {
pubEndPort, err = strconv.Atoi(f.Publicendport)
if err != nil {
return err
}
```
##########
cloudstack/resource_cloudstack_port_forward.go:
##########
@@ -279,15 +295,31 @@ func resourceCloudStackPortForwardRead(d
*schema.ResourceData, meta interface{})
return err
}
+ privEndPort, err := strconv.Atoi(f.Privateendport)
+ if err != nil {
+ return err
+ }
+
pubPort, err := strconv.Atoi(f.Publicport)
if err != nil {
return err
}
+ pubEndPort, err := strconv.Atoi(f.Publicendport)
+ if err != nil {
+ return err
+ }
+
// Update the values
forward["protocol"] = f.Protocol
forward["private_port"] = privPort
forward["public_port"] = pubPort
+ if f.Privateendport != "" && f.Privateendport !=
f.Privateport {
+ forward["private_end_port"] = privEndPort
+ }
+ if f.Publicendport != "" && f.Publicendport !=
f.Publicport {
Review Comment:
These conditions check if the end port strings are non-empty and different
from the start port strings, but the variables `privEndPort` and `pubEndPort`
will contain invalid values (0 and error) when the strings are empty due to the
unconditional parsing above. The parsing should be moved inside these
conditional blocks.
```suggestion
// Parse public port as usual
pubPort, err := strconv.Atoi(f.Publicport)
if err != nil {
return err
}
// Update the values
forward["protocol"] = f.Protocol
forward["private_port"] = privPort
forward["public_port"] = pubPort
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
}
```
--
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
