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

Fixing load balancer lister to not list https lb twice #115

Closed nikhiljindal closed 6 years ago

nikhiljindal commented 6 years ago

Fixes https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/107

Have also refactored the tests to use multiple testcases in a single test rather than rewriting the test each time for each case. Added a test case for https as well.

cc @G-Harmon @csbell @madhusudancs


This change is Reviewable

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 138e32adf22859bbb37ee0caf905ce8c053a1208 on nikhiljindal:listHttps into on GoogleCloudPlatform:master.

G-Harmon commented 6 years ago

LGTM after fixing 2 small suggestions.


Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, 2 unresolved discussions.


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

  }{
      {
          // Empty list when there are no forwarding rules.

Can you add a "description" field to the testCases struct? (And print the description when it runs)


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

false nit: document literals


Comments from Reviewable

nikhiljindal commented 6 years ago

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


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

Previously, G-Harmon wrote…
Can you add a "description" field to the testCases struct? (And print the description when it runs)

Done.


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

Previously, G-Harmon wrote…
> false nit: document literals

Done.


Comments from Reviewable

nikhiljindal commented 6 years ago

Thanks for the review @G-Harmon Updated as per comments.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 62e14b0d64f0938de9d453cb12469fa90d8f72c5 on nikhiljindal:listHttps into on GoogleCloudPlatform:master.