use kube::util::find-binary to find kubectl #1844
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
k8s-ci-robot
added
size/S
mikedanese
force-pushed
the
bzl
branch
3 times, most recently
from
035b1d0 to
29e025d
Compare
hack/ci/e2e-k8s.sh
Outdated
| # This is run from k/k root, so shellcheck shouldn't expect to be able to | ||
| # follow hack/lib/util.sh. | ||
| # shellcheck disable=SC1091 | ||
| kubectl_path="$(source hack/lib/util.sh; kube::util::find-binary '.test')" |
| # TODO: remove this, it was fixed in 1.13+ | ||
| mkdir -p '_output/bin/' | ||
| cp 'bazel-bin/test/e2e/e2e.test' '_output/bin/' | ||
| PATH="$(dirname "$(find "${PWD}/bazel-bin/" -name kubectl -type f)"):${PATH}" |
There was a problem hiding this comment.
why isn't this sufficient? most of the kube::util::find-binary logic is for the makefile output.
is there a non-executable file in bazel-bin named kubectl now?
There was a problem hiding this comment.
I suspect it would work fine to use find right now but I'm not very familiar with how we use the bazel cache and future changes (e.g. cross building in the same CI pool) might subtly cause issues. kube::util::find-binary is more complex but it does a reasonable job. I don't feel too strongly either way. Yet another option is to use cluster/kubectl.sh like other CI jobs.
There was a problem hiding this comment.
Pushed a fourth option that doesn't depend on kube::util::find-binary
There was a problem hiding this comment.
we can't cross build in bazel still because of CGO, we have never run cross build in CI.
locally might be unfortunate though ...
hack/ci/e2e-k8s.sh
Outdated
| @@ -1,4 +1,4 @@ | |||
| #!/bin/sh | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
this was intentionally written in posix sh since the logic is simple.
There was a problem hiding this comment.
Switched in order to source hack/lib/util.sh. Thought process: bash is fairly universal nowadays, used in most places in k/*, and all other shell scripts in this directory use bash. It can still be simple even if it's bash :)
mikedanese
force-pushed
the
bzl
branch
2 times, most recently
from
747fb3a to
f19df36
Compare
|
/retest |
mikedanese
force-pushed
the
bzl
branch
2 times, most recently
from
4d55270 to
f19df36
Compare
To centralize binary selection in k/k. Also drop the e2e.test copying which is no longer needed according to the comment.
| @@ -67,16 +67,16 @@ build_with_bazel() { | |||
| # make sure we have e2e requirements | |||
| bazel build //cmd/kubectl //test/e2e:e2e.test //vendor/github.com/onsi/ginkgo/ginkgo | |||
|
|
|||
| kubectl_path="$(bazel aquery 'mnemonic("GoLink", //cmd/kubectl)' \ | |||
| | grep '^ Outputs: \[.*\]$' \ | |||
| | sed 's/^ Outputs: \[\(.*\)\]$/\1/')" | |||
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, mikedanese 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
approved
To centralize binary selection in k/k. Also drop the e2e.test copying
which is no longer needed according to the comment.