-
Notifications
You must be signed in to change notification settings - Fork 400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable control plane firewall pointer arguments and trim spaces #1555
Enable control plane firewall pointer arguments and trim spaces #1555
Conversation
582d3f8
to
0966af0
Compare
commands/functions.go
Outdated
for _, parm := range parms { | ||
if parm.Init { | ||
keyVal := parm.Key + "=" + parm.Value | ||
for _, param := range params { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by: Fixing some Spell check CI failures https://github.com/digitalocean/doctl/actions/runs/9994130657/job/27623184485?pr=1555
commands/kubernetes.go
Outdated
@@ -1679,6 +1675,9 @@ func buildClusterCreateRequestFromArgs(c *CmdConfig, r *godo.KubernetesClusterCr | |||
if r.ControlPlaneFirewall == nil { | |||
r.ControlPlaneFirewall = &godo.KubernetesControlPlaneFirewall{} | |||
} | |||
for i := range controlPlaneFirewallAllowedAddresses { | |||
controlPlaneFirewallAllowedAddresses[i] = strings.ReplaceAll(controlPlaneFirewallAllowedAddresses[i], " ", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to prefer strings.TrimSpace()
which also handles all sorts of unicode variants.
Similar for the other usage elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've refactored it to use strings.TrimSpace()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the Kubernetes parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM!
enable-control-plane-firewall
flag is a optional boolean and only pass the configuration when needed.