Closed nikhiljindal closed 6 years ago
mostly looks good. just a few minor comments.
Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.
app/kubemci/cmd/delete.go, line 130 at r1 (raw file):
return ingErr } fmt.Printf("%s. Continuing with force delete", ingErr)
Is it really a good idea to continue if UnmarshallAndApplyDefault fails? It just checks the namespaces and the annotations.
app/kubemci/cmd/delete.go, line 144 at r1 (raw file):
return clientsErr } fmt.Printf("%s. Continuing with force delete", clientsErr)
I think you need a newline at the end of Printf()s
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 231 at r1 (raw file):
// DeleteLoadBalancer deletes the GCP resources associated with the L7 GCP load balancer for the given ingress. // TODO(nikhiljindal): Do not require the ingress yaml from users. Just the name should be enough. We can fetch ingress YAML from one of the clusters. func (l *LoadBalancerSyncer) DeleteLoadBalancer(ing *v1beta1.Ingress, forceDelete bool) error {
nit: document new parameters.
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 405 at r1 (raw file):
t.Fatalf("unexpected error in test setup: %s", err) } if err := lbc.CreateLoadBalancer(ing, true /*forceUpdate*/, true /*validate*/, clusters); err != nil {
nit: set force=false while creating?
Comments from Reviewable
Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.
app/kubemci/cmd/delete.go, line 130 at r1 (raw file):
Is it really a good idea to continue if UnmarshallAndApplyDefault fails? It just checks the namespaces and the annotations.
Right, removed. Thanks for pointing that out.
On second thought have also removed continuing from after GetClients. That also just instantiates clients based on kubeconfig.
app/kubemci/cmd/delete.go, line 144 at r1 (raw file):
I think you need a newline at the end of Printf()s
Done
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 231 at r1 (raw file):
nit: document new parameters.
Done
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 405 at r1 (raw file):
nit: set force=false while creating?
Right. Good catch
Comments from Reviewable
Thanks for the review @G-Harmon Updated code as per comments
Reviewed 1 of 2 files at r2, 2 of 3 files at r3. Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.
app/kubemci/cmd/delete.go, line 130 at r1 (raw file):
Right, removed. Thanks for pointing that out. On second thought have also removed continuing from after GetClients. That also just instantiates clients based on kubeconfig.
Did you actually change and push this? it looks the same...
Comments from Reviewable
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.
app/kubemci/cmd/delete.go, line 130 at r1 (raw file):
Did you actually change and push this? it looks the same...
Yes I did push the change. If you go directly to https://beta.reviewable.io/reviews/googlecloudplatform/k8s-multicluster-ingress/168 then you will see the change. If you click on the link for this comment, it takes you to https://beta.reviewable.io/reviews/googlecloudplatform/k8s-multicluster-ingress/168#-L97omfR-zKgXKdxxyDh, which shows an older revision (the one with the comment).
Maybe you clicked on the link and hence are viewing an older revision?
Comments from Reviewable
Reviewed 1 of 3 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.
app/kubemci/cmd/delete.go, line 130 at r1 (raw file):
Yes I did push the change. If you go directly to https://beta.reviewable.io/reviews/googlecloudplatform/k8s-multicluster-ingress/168 then you will see the change. If you click on the link for this comment, it takes you to https://beta.reviewable.io/reviews/googlecloudplatform/k8s-multicluster-ingress/168#-L97omfR-zKgXKdxxyDh, which shows an older revision (the one with the comment). Maybe you clicked on the link and hence are viewing an older revision?
I thought I had changed it to look at the latest rev, but I think I was actually comparing r1 to r2. oops.
Comments from Reviewable
Fixes https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/164
Adding a
--force
flag to kubemci delete to continue deletion even in case of errors.Will add an e2e test as part of remove-clusters test (delete the mci after it has been removed from all clusters)
cc @G-Harmon
This change is![Reviewable](https://reviewable.io/review_button.svg)