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

--validate: Check server version for minimum version number. #167

Closed G-Harmon closed 6 years ago

G-Harmon commented 6 years ago

We need a minimum of v1.8.1 for MCI to work.

This is for Issue #53 .

cc @nikhiljindal


This change is Reviewable

G-Harmon commented 6 years ago

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.


app/kubemci/pkg/validations/validations.go, line 125 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…
This never returns an error so no need to include it in method signatures

Done.


app/kubemci/pkg/validations/validations_test.go, line 38 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…
Add v1.9.3-gke.0 as a test case. Also one without "v". Like 1.2.3

Are you sure it's valid to not start with "v"? Right now, I'm including that in my regular expression: ^v([0-9]).([0-9]).([0-9]*)

Added gke version string.


app/kubemci/pkg/validations/validations_test.go, line 79 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…
optional: You can link to https://github.com/kubernetes/ingress-gce/issues/182 in case reader is curious

Done.


app/kubemci/pkg/validations/validations_test.go, line 94 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…
nit: line break not needed

Done.


Comments from Reviewable

nikhiljindal commented 6 years ago

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.


app/kubemci/pkg/validations/validations_test.go, line 38 at r1 (raw file):

Previously, G-Harmon wrote…
Are you sure it's valid to not start with "v"? Right now, I'm including that in my regular expression: ^v([0-9]*).([0-9]*).([0-9]*) Added gke version string.

invalid is fine. i was pointing out that you dont have any case without the v prefix


Comments from Reviewable

G-Harmon commented 6 years ago

PTAL, I think this is ready to merge. I added a TODO to move the NodePort validation out of loadbalancersyncer.go and into create.go

nikhiljindal commented 6 years ago

/lgtm thanks!