DaanHoogland commented on code in PR #242:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/242#discussion_r2417020118
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -491,76 +485,126 @@ func resourceCloudStackNetworkACLRuleRead(d
*schema.ResourceData, meta interface
// Delete the known rule so only unknown rules
remain in the ruleMap
delete(ruleMap, id.(string))
- // Create a set with all CIDR's
- cidrs := &schema.Set{F: schema.HashString}
+ // Create a list with all CIDR's
+ var cidrs []interface{}
for _, cidr := range strings.Split(r.Cidrlist,
",") {
- cidrs.Add(cidr)
+ cidrs = append(cidrs, cidr)
}
// Update the values
rule["action"] = strings.ToLower(r.Action)
rule["protocol"] = r.Protocol
rule["traffic_type"] =
strings.ToLower(r.Traffictype)
rule["cidr_list"] = cidrs
- rules.Add(rule)
+ rules = append(rules, rule)
log.Printf("[DEBUG] Added ALL rule to state:
%+v", rule)
}
if rule["protocol"].(string) == "tcp" ||
rule["protocol"].(string) == "udp" {
- ps, ok := rule["ports"].(*schema.Set)
- if !ok || ps == nil {
- log.Printf("[DEBUG] No ports specified
for TCP/UDP rule, initializing empty set")
- ps = &schema.Set{F: schema.HashString}
- }
+ // Check for deprecated ports field first (for
backward compatibility)
+ ps, hasPortsSet := rule["ports"].(*schema.Set)
+ portStr, hasPort := rule["port"].(string)
+
+ if hasPortsSet && ps.Len() > 0 {
+ // Handle deprecated ports field
(multiple ports)
+ log.Printf("[DEBUG] Processing %d ports
for TCP/UDP rule (deprecated field)", ps.Len())
+
+ var ports []interface{}
+ for _, port := range ps.List() {
+ id, ok := uuids[port.(string)]
+ if !ok {
+ log.Printf("[DEBUG] No
UUID for port %s, skipping", port.(string))
+ continue
+ }
+
+ r, ok := ruleMap[id.(string)]
+ if !ok {
+ log.Printf("[DEBUG]
TCP/UDP rule for port %s with ID %s not found, removing UUID", port.(string),
id.(string))
+ delete(uuids,
port.(string))
+ continue
+ }
+
+ // Delete the known rule so
only unknown rules remain in the ruleMap
+ delete(ruleMap, id.(string))
+
+ var cidrs []interface{}
+ for _, cidr := range
strings.Split(r.Cidrlist, ",") {
+ cidrs = append(cidrs,
cidr)
+ }
+
+ rule["action"] =
strings.ToLower(r.Action)
+ rule["protocol"] = r.Protocol
+ rule["traffic_type"] =
strings.ToLower(r.Traffictype)
+ rule["cidr_list"] = cidrs
+ ports = append(ports, port)
+ log.Printf("[DEBUG] Added port
%s to TCP/UDP rule", port.(string))
+ }
+
+ rule["ports"] =
schema.NewSet(schema.HashString, ports)
+ rules = append(rules, rule)
+ log.Printf("[DEBUG] Added TCP/UDP rule
with deprecated ports to state: %+v", rule)
- // Create an empty schema.Set to hold all ports
- ports := &schema.Set{F: schema.HashString}
- log.Printf("[DEBUG] Processing %d ports for
TCP/UDP rule", ps.Len())
+ } else if hasPort && portStr != "" {
+ log.Printf("[DEBUG] Processing single
port for TCP/UDP rule: %s", portStr)
- // Loop through all ports and retrieve their
info
- for _, port := range ps.List() {
- id, ok := uuids[port.(string)]
+ id, ok := uuids[portStr]
if !ok {
- log.Printf("[DEBUG] No UUID for
port %s, skipping", port.(string))
+ log.Printf("[DEBUG] No UUID for
port %s, skipping rule", portStr)
continue
}
- // Get the rule
r, ok := ruleMap[id.(string)]
if !ok {
- log.Printf("[DEBUG] TCP/UDP
rule for port %s with ID %s not found, removing UUID", port.(string),
id.(string))
- delete(uuids, port.(string))
+ log.Printf("[DEBUG] TCP/UDP
rule for port %s with ID %s not found, removing UUID", portStr, id.(string))
+ delete(uuids, portStr)
continue
}
// Delete the known rule so only
unknown rules remain in the ruleMap
delete(ruleMap, id.(string))
- // Create a set with all CIDR's
- cidrs := &schema.Set{F:
schema.HashString}
+ var cidrs []interface{}
for _, cidr := range
strings.Split(r.Cidrlist, ",") {
- cidrs.Add(cidr)
+ cidrs = append(cidrs, cidr)
}
- // Update the values
rule["action"] =
strings.ToLower(r.Action)
rule["protocol"] = r.Protocol
rule["traffic_type"] =
strings.ToLower(r.Traffictype)
rule["cidr_list"] = cidrs
- ports.Add(port)
- log.Printf("[DEBUG] Added port %s to
TCP/UDP rule", port.(string))
- }
+ rule["port"] = portStr
+ rules = append(rules, rule)
+ log.Printf("[DEBUG] Added TCP/UDP rule
with single port to state: %+v", rule)
- // If there is at least one port found, add
this rule to the rules set
- if ports.Len() > 0 {
- rule["ports"] = ports
- rules.Add(rule)
- log.Printf("[DEBUG] Added TCP/UDP rule
to state: %+v", rule)
} else {
- // Add the rule even if no ports are
specified, as ports are optional
- rule["ports"] = ports
- rules.Add(rule)
- log.Printf("[DEBUG] Added TCP/UDP rule
with no ports to state: %+v", rule)
+ log.Printf("[DEBUG] Processing TCP/UDP
rule with no port specified")
+
+ id, ok := uuids["all_ports"]
+ if !ok {
+ log.Printf("[DEBUG] No UUID for
all_ports, skipping rule")
+ continue
+ }
+
+ r, ok := ruleMap[id.(string)]
+ if !ok {
+ log.Printf("[DEBUG] TCP/UDP
rule for all_ports with ID %s not found, removing UUID", id.(string))
+ delete(uuids, "all_ports")
+ continue
+ }
+
+ delete(ruleMap, id.(string))
+
+ var cidrs []interface{}
+ for _, cidr := range
strings.Split(r.Cidrlist, ",") {
+ cidrs = append(cidrs, cidr)
+ }
+
+ rule["action"] =
strings.ToLower(r.Action)
+ rule["protocol"] = r.Protocol
+ rule["traffic_type"] =
strings.ToLower(r.Traffictype)
+ rule["cidr_list"] = cidrs
+ rules = append(rules, rule)
+ log.Printf("[DEBUG] Added TCP/UDP rule
with no port to state: %+v", rule)
Review Comment:
new method?
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -149,12 +155,12 @@ func resourceCloudStackNetworkACLRuleCreate(d
*schema.ResourceData, meta interfa
}
// Create all rules that are configured
Review Comment:
@CodeBleu , this is an example of what I meant earlier. The comment above
can be the name of a method that contains the if-else block below. That would
make it easier to read and maintain. Again, this is not due to this PR but
changing it would make the code more readable.
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -491,76 +485,126 @@ func resourceCloudStackNetworkACLRuleRead(d
*schema.ResourceData, meta interface
// Delete the known rule so only unknown rules
remain in the ruleMap
delete(ruleMap, id.(string))
- // Create a set with all CIDR's
- cidrs := &schema.Set{F: schema.HashString}
+ // Create a list with all CIDR's
+ var cidrs []interface{}
for _, cidr := range strings.Split(r.Cidrlist,
",") {
- cidrs.Add(cidr)
+ cidrs = append(cidrs, cidr)
}
// Update the values
rule["action"] = strings.ToLower(r.Action)
rule["protocol"] = r.Protocol
rule["traffic_type"] =
strings.ToLower(r.Traffictype)
rule["cidr_list"] = cidrs
- rules.Add(rule)
+ rules = append(rules, rule)
log.Printf("[DEBUG] Added ALL rule to state:
%+v", rule)
}
if rule["protocol"].(string) == "tcp" ||
rule["protocol"].(string) == "udp" {
- ps, ok := rule["ports"].(*schema.Set)
- if !ok || ps == nil {
- log.Printf("[DEBUG] No ports specified
for TCP/UDP rule, initializing empty set")
- ps = &schema.Set{F: schema.HashString}
- }
+ // Check for deprecated ports field first (for
backward compatibility)
Review Comment:
probably two or three even.
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -491,76 +485,126 @@ func resourceCloudStackNetworkACLRuleRead(d
*schema.ResourceData, meta interface
// Delete the known rule so only unknown rules
remain in the ruleMap
delete(ruleMap, id.(string))
- // Create a set with all CIDR's
- cidrs := &schema.Set{F: schema.HashString}
+ // Create a list with all CIDR's
+ var cidrs []interface{}
for _, cidr := range strings.Split(r.Cidrlist,
",") {
- cidrs.Add(cidr)
+ cidrs = append(cidrs, cidr)
}
// Update the values
rule["action"] = strings.ToLower(r.Action)
rule["protocol"] = r.Protocol
rule["traffic_type"] =
strings.ToLower(r.Traffictype)
rule["cidr_list"] = cidrs
- rules.Add(rule)
+ rules = append(rules, rule)
log.Printf("[DEBUG] Added ALL rule to state:
%+v", rule)
}
if rule["protocol"].(string) == "tcp" ||
rule["protocol"].(string) == "udp" {
- ps, ok := rule["ports"].(*schema.Set)
- if !ok || ps == nil {
- log.Printf("[DEBUG] No ports specified
for TCP/UDP rule, initializing empty set")
- ps = &schema.Set{F: schema.HashString}
- }
+ // Check for deprecated ports field first (for
backward compatibility)
Review Comment:
can this code block go into a new method?
--
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]
