kubemci list command: look for relevant global forwarding rules. - Githubissues

GoogleCloudPlatform / k8s-multicluster-ingress

kubemci: Command line tool to configure L7 load balancers using multiple kubernetes clusters

Apache License 2.0

377 stars 68 forks source link

kubemci list command: look for relevant global forwarding rules. #91

Closed G-Harmon closed 6 years ago

G-Harmon commented 6 years ago

cc @nikhiljindal @csbell @madhusudancs

This change is

k8s-ci-robot commented 6 years ago

Hi @G-Harmon. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

G-Harmon commented 6 years ago

This can't be merged until https://github.com/kubernetes/ingress-gce/pull/78 is merged, AND we re-vendor that commit into this repo.

nikhiljindal commented 6 years ago

/ok-to-test

G-Harmon commented 6 years ago

The test failure is expected here, since it doesn't have the updates I made to ingress-gce.

nikhiljindal commented 6 years ago

Thanks @G-Harmon looks great, main comment is about who should do formatting for printing.

Also needs rebase with #93

coveralls commented 6 years ago

Changes Unknown when pulling c0505ddc6ceba7d90dcea0a525e3623446d6d6b8 on G-Harmon:mciList into on GoogleCloudPlatform:master.

G-Harmon commented 6 years ago

PTAL. sorry it was a little messy, I was a bit rushed last night. I wanted to get your review comments, so thanks for looking at it!

Review status: 0 of 10 files reviewed at latest revision, 12 unresolved discussions.

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 133 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Returned error and printed error should be same? ``` err = fmt.Errorf("Error getting global forwarding rules: %s", err) fmt.Println(err) return err ```

I kind of go back and forth on this point, but I think you're right. (The only downside is that we will likely print the exact same error twice. That's probably better than printing 2 slightly different messages for the same problem.)

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 136 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

What do you mean? Filter out nil rules?

When I first started using the Fake, I had the Fake's ListGlobalForwardingRules returning nil. That crashed this code. I haven't seen a nil return from the real one.

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 141 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Shouldnt we return this error as well?

sure. (I was thinking better to get partial status than none.) Now I return "result" and the error. Is that an okay pattern?

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go, line 118 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

You meant TestGetForwardingRules?

now it's TestListLoadBalancerStatuses.

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go, line 139 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

"unexpected name, expected: %s, actual: %s"

oops, I meant to come back to these Errorfs, but forgot! Done.

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go, line 142 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

same as above

Done.

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go, line 211 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

load balancers -> forwarding rules :)

Done.

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go, line 214 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

nit: add space after :

Done.

app/kubemci/pkg/gcp/forwardingrule/interfaces.go, line 32 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

How about calling this ListLoadBalancerStatuses? From GetForwardingRules, I expect it to return compute.ForwardingRule.

yea, done.

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 223 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

how about calling this formatLoadBalancersList? This method does not know that load balancer statuses came from forwarding rules and hence should have nothing to do with forwarding rules.

Done. makes sense.

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 225 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

call it multicluster ingresses instead of multicluster loadbalancers (since we call them mci's everywhere)?

right :)

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 227 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

unintended?

Done. (removed)

Comments from Reviewable

coveralls commented 6 years ago

Changes Unknown when pulling 58c381299e5d9426e9f15bf5630ed6868bf8023f on G-Harmon:mciList into on GoogleCloudPlatform:master.

nikhiljindal commented 6 years ago

Thanks for the fixes, looks great! Added 2 comments

G-Harmon commented 6 years ago

test failed due to trivial merge conflict. I fixed it and pushed an update.

coveralls commented 6 years ago

Changes Unknown when pulling 6dcc5b29efdbdf5fd2c9e9dc41de06db6937db5f on G-Harmon:mciList into on GoogleCloudPlatform:master.

nikhiljindal commented 6 years ago

/lgtm, thanks!