feat(https://github.com/k3d-io/k3d/issues/1570): introducing the --port-delete flag; #1609
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new --port-delete flag for the k3d cluster edit command, allowing users to remove port mappings from existing clusters. The implementation extends the existing port configuration system to support both addition and removal operations.
Key changes include:
- Added a new
Removalfield toPortWithNodeFilterstype to differentiate between add and delete operations - Refactored port mapping logic to support both addition and removal through unified functions
- Added comprehensive support for removing port configurations from both nodes and load balancers
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/util.go | Added generic RemoveFirst utility function for removing elements from slices |
| pkg/config/v1alpha5/types.go | Extended PortWithNodeFilters struct with Removal field |
| pkg/client/ports.go | Refactored port mapping functions to support both add and remove operations |
| pkg/client/loadbalancer.go | Added load balancer port removal functionality and improved error messages |
| pkg/client/cluster.go | Fixed format specifier in error message |
| cmd/cluster/clusterEdit.go | Added --port-delete flag and refactored command parsing |
Comments suppressed due to low confidence (2)
pkg/client/loadbalancer.go:250
- The append operation on line 249 is executed inside the loop over existing names, which means it will append the nodename multiple times (once for each existing name that doesn't match). This should be moved outside the inner loop or use a different approach to check if the nodename already exists.
for _, existingName := range loadbalancer.Config.Ports[portconfig] {
if nodename == existingName {
continue nodenameLoop
}
loadbalancer.Config.Ports[portconfig] = append(loadbalancer.Config.Ports[portconfig], nodename)
}
pkg/client/ports.go:101
- The error message 'error adding port mappings' is misleading when performing removal operations. It should be updated to reflect the actual operation being performed, e.g., 'error processing port mappings'.
} else if suffix != util.NodeFilterMapKeyAll {
return fmt.Errorf("error adding port mappings: unknown suffix %s", suffix)
}
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| nodenameLoop: | ||
| for _, nodename := range nodenames { | ||
| for _, existingNames := range loadbalancer.Config.Ports[portconfig] { | ||
| if nodename == existingNames { | ||
| for _, existingName := range loadbalancer.Config.Ports[portconfig] { | ||
| if nodename == existingName { | ||
| continue nodenameLoop | ||
| } |
There was a problem hiding this comment.
The append operation is incorrectly placed inside the inner loop. It should be moved outside the inner loop to avoid adding the same nodename multiple times when there are existing entries.
Copilot uses AI. Check for mistakes.
| info "Checking port-mappings..." | ||
| docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qE "^$existingPortMappingContainerPort" || failed "failed to verify pre-existing port-mapping" | ||
| docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qE "^$newPortMappingContainerPort" || failed "failed to verify pre-existing port-mapping" | ||
| docker inspect k3d-$clustername-serverlb --format '{{ range $k, $v := .NetworkSettings.Ports }}{{ printf "%s->%s\n" $k $v }}{{ end }}' | grep -qE "^$newPortMappingContainerPort" || failed "failed to verify new port-mapping" |
There was a problem hiding this comment.
[nitpick] The error message says 'failed to verify new port-mapping' but this should be 'failed to verify added port-mapping' to be more specific about what was being verified.
Copilot uses AI. Check for mistakes.
|
Thank you very much for this addition! |
2 participants
cluster editcommand.PortWithNodeFilterstype was already quite case-specific one, so i figured adding the flag there to carry it deeper into the code was acceptable.