Update code to cache correct images for different k8s versions #2849
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mlgibbons Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
|
Can one of the admins verify this patch? |
|
/assign @aaron-prindle |
pkg/minikube/constants/constants.go
Outdated
| "k8s.gcr.io/k8s-dns-kube-dns-amd64:1.14.8", | ||
| "k8s.gcr.io/k8s-dns-dnsmasq-nanny-amd64:1.14.8", | ||
| "k8s.gcr.io/k8s-dns-sidecar-amd64:1.14.8", | ||
| v1_10, _ := semver.ParseRange(">=1.10.0 <1.11.0") |
There was a problem hiding this comment.
Can you use MustParse here instead of dropping the errors?
pkg/minikube/constants/constants.go
Outdated
| "k8s.gcr.io/kube-scheduler-amd64:" + version, | ||
| "k8s.gcr.io/kube-controller-manager-amd64:" + version, | ||
| "k8s.gcr.io/kube-apiserver-amd64:" + version, | ||
| if v1_10(kubernetesVersion) { |
There was a problem hiding this comment.
nit: I usually prefer a switch statement to a long list of if elses.
There was a problem hiding this comment.
I agree. With the multiple booleans though I'm not sure how I could structure this more cleanly so settled on that code structure as being clean and understandable.
There was a problem hiding this comment.
Ah, SGTM then. I'll think on this a bit and see if I can come up with a way to make it more declarative longer term.
pkg/minikube/constants/constants.go
Outdated
| "k8s.gcr.io/kube-scheduler-amd64:" + version, | ||
| "k8s.gcr.io/kube-controller-manager-amd64:" + version, | ||
| "k8s.gcr.io/kube-apiserver-amd64:" + version, | ||
| if v1_10(kubernetesVersion) { |
There was a problem hiding this comment.
Ah, SGTM then. I'll think on this a bit and see if I can come up with a way to make it more declarative longer term.
pkg/minikube/constants/constants.go
Outdated
| "k8s.gcr.io/kube-scheduler-amd64:" + version, | ||
| "k8s.gcr.io/kube-controller-manager-amd64:" + version, | ||
| "k8s.gcr.io/kube-apiserver-amd64:" + version, | ||
| if v1_10(kubernetesVersion) { |
There was a problem hiding this comment.
One last nit, it might be a bit more clear if this first if statement is something like:
if v_10 || greater_than_v_10
that makes it clearer that versions outside the range (but higher) get the same images as the latest.
|
/cc dlorenc |
|
@minikube-bot OK to test |
|
I'm not quite clear on the process for merging but I'm wondering why this is sitting and not merged yet? |
See #2827