search

GoogleCloudPlatform / k8s-multicluster-ingress

kubemci: Command line tool to configure L7 load balancers using multiple kubernetes clusters
Apache License 2.0
376 stars 68 forks source link

Update ingress with --force #110

Closed glindstedt closed 6 years ago

glindstedt commented 6 years ago

This will fix #109. I've refactored the ingress creation and deletion into yet another syncer. Let me know if you'd like me to take another path.

Things to do before PR is complete:

k8s-ci-robot commented 6 years ago

Hi @glindste. Thanks for your PR.

I'm waiting for a kubernetes or GoogleCloudPlatform 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.
nikhiljindal commented 6 years ago

/ok-to-test

cc @csbell @G-Harmon

nikhiljindal commented 6 years ago

I like moving out the code to create/delete ingresses from cmd/ and putting it into a library with a formal interface. But I dont think that that library needs to be in gcp/, since the logic is load balancer provider-agnostic.

Today we have only one LoadBalancerSyncer (the gcp one), but the idea is that we will have many for different load balancer providers. The generic cmd code will do the generic parts (like create ingress in all clusters) and then call the provider specific LoadBalancerSyncer to do provider specific bits.

To continue with that approach, how about moving IngressSyncer to app/kubemci/pkg/ingress/, instead of app/kubemci/pkg/gcp/ingress/. And then calling it directly from cmd/create.go and delete.go before calling LoadBalancerSyncer? That way we get a library as we want and gcp LoadBalancerSyncer continues to have only gcp specific parts. And when we add more LoadBalancerSyncers they continue to use the generic cmd/ code.

glindstedt commented 6 years ago

Great point about separating ingress handling from the gcp package, it didn't occur to me that more LB providers would be added. I'll apply your suggestions.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 65651c7464a88225559e5f2161e40eaa68ddd17c on glindste:update-ingress into on GoogleCloudPlatform:master.

nikhiljindal commented 6 years ago

Thanks @glindste This looks great! Remaining comments are about adding comments for public methods and interfaces and adding a test.

Feel free to squash commits

glindstedt commented 6 years ago

I've addressed your comments, implemented tests and squashed into one commit. Let me know if there's anything more to fix!

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 4507283292585a5f41a258f4ca8944ceceb5fa4d on glindste:update-ingress into on GoogleCloudPlatform:master.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 4507283292585a5f41a258f4ca8944ceceb5fa4d on glindste:update-ingress into on GoogleCloudPlatform:master.

nikhiljindal commented 6 years ago

Thanks @glindste Sorry for the delay, I missed your email about the update.

The comment about the tests is optional, but we need to fix the comparison.

nikhiljindal commented 6 years ago

Any update @glindste?

glindstedt commented 6 years ago

Sorry @nikhiljindal, I haven't been able to work on this for a while, but I should be able to fix your comments today.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 354b1a0c710c98e14eacbfcfb3d4bc0bd947ebef on glindste:update-ingress into on GoogleCloudPlatform:master.

glindstedt commented 6 years ago

I've at least fixed the comparison, I opted to copy the implementation from the federation repo because I didn't want to add an extra dependency just for that. I also modified one of the tests to break if we regress, not sure if that's the right place to test it though.

If you want to merge ASAP I could just squash (assuming you have no more comments) and do the test refactoring in a separate PR.

nikhiljindal commented 6 years ago

Thanks @glindste The changes look good to me. The test to break if we regress looks good too.

I dont have any comments. If you can do the test refactoring in next few days, then we can wait to merge this PR. Else, we can add a TODO for now and do it in a follow up PR. I dont have a real hurry, but would love to see this merge soon :)

glindstedt commented 6 years ago

Ok! I'll see if I can get to refactoring the tests in the next couple of days, otherwise I'll just add a TODO and squash and let you know you can merge :)

nikhiljindal commented 6 years ago

Sounds good

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 1265a40f3c1b723c1a8262df9572253785ac006f on glindste:update-ingress into on GoogleCloudPlatform:master.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 1265a40f3c1b723c1a8262df9572253785ac006f on glindste:update-ingress into on GoogleCloudPlatform:master.

glindstedt commented 6 years ago

I refactored the tests, though they're a bit ugly due to the reactor actions. Let me know if you have any comments.

nikhiljindal commented 6 years ago

Thanks @glindste I would have kept the delete test different. But its not a big issue for me.

/lgtm.