Closed nikhiljindal closed 6 years ago
I looked at the non *_test.go files so far have a few questions about that.
Reviewed 5 of 8 files at r1. Review status: 5 of 8 files reviewed at latest revision, all discussions resolved.
app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go, line 85 at r1 (raw file):
} else { // Mark the link as removed. removeLinks[ig] = false
Can v.IGLinks have duplicates? If not, then this code doesn't look like it does anything.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 361 at r1 (raw file):
sd != nil why the check here? Overall, this logic seems complex/fragile. partially, I don't quite follow what you're doing here.
Comments from Reviewable
Review status: 5 of 8 files reviewed at latest revision, 2 unresolved discussions.
app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go, line 85 at r1 (raw file):
Can v.IGLinks have duplicates? If not, then this code doesn't look like it does anything.
Yes our tests have duplicates. Added a comment to make that explicit
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 361 at r1 (raw file):
> sd != nil why the check here? Overall, this logic seems complex/fragile. partially, I don't quite follow what you're doing here.
Updated the variable names to clarify this as discussed.
Comments from Reviewable
Thanks for the review @G-Harmon Updated as per comments. PTAL
I didn't quite follow the changes in loadbalancersyncer_test.go. I put questions there in that part of the patch.
Reviewed 3 of 8 files at r1, 2 of 2 files at r2. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 352 at r2 (raw file):
Sd Why is this called sd? It returns a LoadBalancerStatus...
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 369 at r2 (raw file):
Why do you remove some checks? like health checks, url maps, target proxies, forwarding rules?
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 426 at r2 (raw file):
if !reflect.DeepEqual(fw.IGLinks, expectedFWIGLinks) { t.Errorf("unexpected IG links on firewall rule, expected: %v, got: %v", expectedFWIGLinks, fw.IGLinks) }
(Why don't you check as many things as before? like forwarding rules)
Comments from Reviewable
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 352 at r2 (raw file):
>Sd Why is this called sd? It returns a LoadBalancerStatus...
sd for status description :) Renamed to status.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 369 at r2 (raw file):
> ``` > fhc := lbc.hcs.(*healthcheck.FakeHealthCheckSyncer) > if len(fhc.EnsuredHealthChecks) == 0 { > t.Errorf("unexpected health checks not created") > } > ``` Why do you remove some checks? like health checks, url maps, target proxies, forwarding rules?
They are not affected by RemoveFromClusters. Those were essentially testing that CreateLoadBalancer is working as expected, which is duplicated in the relevant test already.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 426 at r2 (raw file):
> ``` > if !reflect.DeepEqual(fw.IGLinks, expectedFWIGLinks) { > t.Errorf("unexpected IG links on firewall rule, expected: %v, got: %v", expectedFWIGLinks, fw.IGLinks) > } > ``` (Why don't you check as many things as before? like forwarding rules)
Same as above. Here am limiting the checks to resources that are affected by RemoveFromClusters.
Comments from Reviewable
Updated code as per comments. @G-Harmon PTAL
Feel free to merge as is, or add the one more tiny check.
Reviewed 2 of 2 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 369 at r2 (raw file):
They are not affected by RemoveFromClusters. Those were essentially testing that CreateLoadBalancer is working as expected, which is duplicated in the relevant test already.
ah, okay, sounds good.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 452 at r3 (raw file):
be := fbs.EnsuredBackendServices[0]
Do we care about verifying both? I think I submitted this shortcoming, so totally optional.
Comments from Reviewable
Review status: all files reviewed at latest revision, 1 unresolved discussion.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 452 at r3 (raw file):
> ``` > be := fbs.EnsuredBackendServices[0] > ``` Do we care about verifying both? I think I submitted this shortcoming, so totally optional.
Good catch. Updated to verify both
Comments from Reviewable
Thanks for the review @G-Harmon Updated code as per comments.
Will merge once tests pass.
Reviewed 2 of 2 files at r4. Review status: all files reviewed at latest revision, 1 unresolved discussion.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 452 at r3 (raw file):
Good catch. Updated to verify both
thanks!
Comments from Reviewable
Ref https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/58 and https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/145
Fixing TODOs from https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/pull/151 and https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/pull/149
Adding more unit tests for remove-clusters command. Bulk of the PR is unit test changes, with minor bug fixes.
cc @csbell @G-Harmon @madhusudancs @perotinus
This change is![Reviewable](https://reviewable.io/review_button.svg)