search

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

Add --force support for URL Map. #70

Closed G-Harmon closed 6 years ago

G-Harmon commented 6 years ago

Compares existing and desired UrlMaps when one already exists. Only changes an existing one if the forceUpdate parameter is passed in.

cc @nikhiljindal @csbell @madhusudancs

This change is Reviewable

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.
nikhiljindal commented 6 years ago

/ok-to-test

nikhiljindal commented 6 years ago

Thanks mostly looks great, few comments

G-Harmon commented 6 years ago

Agreed, but urlMapMatches is mutating already.

I think this is different because this is required not really for matching (we could set existingUM.Fingerprint="" for that), but this is required for updating right? Will the update fail if we do not set Fingerprint? Say that in the comment if thats true.

Yes, we have to include the fingerprint field. (otherwise we get the error "googleapi: Error 400: Required field 'resource.fingerprint' not specified, required"). I added a comment.

I think it'd be really confusing to have "urlMapMatches" also update the desiredUrlMap. If you really want to put that logic inside that function, then I think we should rename it, like "updateUrlMapAndMatches". I think the way it stands is reasonable. (or another option would be to break up the s.desiredUrlMap method- first get the desired name, then look for an existing one, then compute the rest of the desired url map, passing in the existing one.)

G-Harmon commented 6 years ago

/retest

G-Harmon commented 6 years ago

Friendly ping?

nikhiljindal commented 6 years ago

sounds good about urlMapMatches.

About fingerprint, didnt realise that it needs to be different. Can you update the comment to be explicit about that? Probably also link to documentation that says that?

G-Harmon commented 6 years ago

About fingerprint, didnt realise that it needs to be different. Can you update the comment to be explicit about that? Probably also link to documentation that says that?

sure, done!

nikhiljindal commented 6 years ago

/lgtm, thx!