GKE: Adjust the PATH to the right kubectl for gcloud #3092
Conversation
| @@ -64,6 +68,7 @@ locations=( | |||
| "${KUBE_ROOT}/_output/local/bin/${host_os}/${host_arch}/kubectl" | |||
| "${KUBE_ROOT}/platforms/${host_os}/${host_arch}/kubectl" | |||
| "${HOME}/google-cloud-sdk/bin/kubectl" | |||
| "/usr/local/share/google/google-cloud-sdk/bin/kubectl" | |||
There was a problem hiding this comment.
If we want to force gcloud to pick up the built version, we should remove this line (and the line above) so that the tests will fail on gke if we can't built kubectl. Otherwise we have a weird situation where sometimes we are using the kubectl built from source and other times we are using the kubectl packaged with gcloud.
There was a problem hiding this comment.
Yeah, I actually debated that. We can either force the git version, or allow the gcloud version. As written, this would actually let you do a "make clean" and use the gcloud version for e2e's, though, and there's no easy way to do that otherwise.
There was a problem hiding this comment.
Well, right now if you do a make clean the tests will fail because there won't be a kubecfg to execute, but hopefully that will be fixed soon.
Since users can choose their own google-cloud-sdk install directory, it seems fragile to start encoding common ones here. How about replacing these two lines with $(which kubectl) which will pick it up from the gcloud install directory?
There was a problem hiding this comment.
Actually, better yet, since we're allowing GCLOUD=foo/bar/gcloud, why not KUBECTL=foo/bar/kubectl? Seems like that would allow you to test from a system install, but it forces you to be conscious about it.
There was a problem hiding this comment.
That will work for any installed gcloud, but won't work for a gcloud built from head, because kubectl isn't in the same directory until the tool in packaged.
zmerlynn
force-pushed
the
gke-kubectl-path
branch
from
69ede0b to
117cfd9
Compare
|
@roberthbailey: PTAL, reassigned since you took a look already. :) This implements my last suggestion. |
|
I'm looking too :-) |
|
By "POLA" do you mean "principle of least authority" or "principle of least astonishment"? |
zmerlynn
force-pushed
the
gke-kubectl-path
branch
from
117cfd9 to
612b3c6
Compare
|
I meant least astominshment. I surprised me that I needed to hit the PATH on the Jenkins box to actually influence the |
| @@ -22,6 +22,10 @@ KUBE_ROOT=$(dirname "${BASH_SOURCE}")/.. | |||
| source "${KUBE_ROOT}/cluster/kube-env.sh" | |||
| source "${KUBE_ROOT}/cluster/${KUBERNETES_PROVIDER}/util.sh" | |||
|
|
|||
| get_absolute_dirname() { | |||
|
Thanks for taking a stab at this, Zach. I had implemented the original solution before I was piping |
…rride Force kubectl to the git repo version if we detect a built version. Add a KUBECTL_PATH override so that system/home directory versions of kubectl can be used if desired. Remove existing $HOME inference.
zmerlynn
force-pushed
the
gke-kubectl-path
branch
from
612b3c6 to
e71798d
Compare
|
@mbforbes @roberthbailey: PTAL. It turns out that |
|
Another option is to match config-common.sh and call it |
| } >&2 | ||
| exit 1 | ||
| fi | ||
| elif [[ ! -x "${KUBECTL_PATH}" ]]; then |
There was a problem hiding this comment.
This is probably overkill, but it would be a bit more resilient to actually run kubectl (similarly to how you run gcloud preview in pr #3082) rather than just check the executable bit.
There was a problem hiding this comment.
I think that's a verify_prereqs thing to do, not an every-time-you-run kubectl.sh thing to do. :) I did consider it, though.
|
LGTM. Kicking travis. |
roberthbailey
added
the
lgtm
|
LGTM |
GKE: Adjust the PATH to the right kubectl for gcloud
|
I haven't read this PR so forgive this possibly silly question: is it now possible to run e2e against GKE in all of (old client, new cluster), (new client, old cluster), and (new client, new cluster) configurations? |
|
I think we've only tested the (new client, old cluster) config. The others should at least run (simply because we always pass |
Force kubectl to the git repo version if we detect a built version,
otherwise a system version. Previously, your actual PATH influenced
what kubectl was getting picked up, which violates POLA for a dev
environment.
Along the way: Add the root install of kubectl.