implement LoadBalancer task for ALICloud #5207
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
geojaz
added
area/networking
do-not-merge/work-in-progress
| sourceItems := "" | ||
| for _, cidr := range b.Cluster.Spec.KubernetesAPIAccess { | ||
| if cidr != "0.0.0.0" && cidr != "0.0.0.0/0" { | ||
| sourceItems = sourceItems + cidr + "," |
There was a problem hiding this comment.
I think this leaves a trailing comma; not sure if that's a problem. It might be easier to build a string slice and then join it. Something like:
var cidrs []string
for _, cidr := range ... {
cidrs = append(cidrs, cidr)
}
sourceItems := strings.Join(cidrs, ",")
There was a problem hiding this comment.
Edit: looks like it doesn't matter but leaving as this approach is probably easier & a bit cleaner.
| } | ||
| tags, _, err := cloud.SlbClient().DescribeTags(describeTagsArgs) | ||
| if err != nil { | ||
| glog.V(4).Info("Error getting tags on loadbalancerID:%q", *actual.LoadbalancerId) |
There was a problem hiding this comment.
I think this probably should return an error
|
So this LGTM - one comment on error handling but as this is additive I think this can go in and we can iterate on it! /approve |
k8s-ci-robot
added
lgtm
k8s-ci-robot
removed
the
lgtm
|
@justinsb Thanks for your review. We modified this code, and can we have a new lgtm? |
…e conflict with the upstream branch
|
@justinsb This pr has been a long time, hope it can be merged. |
|
Sorry for the delay @LilyFaFa /approve |
k8s-ci-robot
added
the
lgtm
justinsb
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, LilyFaFa The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Implement LoadBalancer task for AlibabaCloud
This is part of #4127.