add BAZEL_VERSION env to kubekins, check it in caching config #8516
Conversation
k8s-ci-robot
added
cncf-cla: yes
| } | ||
|
|
||
| # look up a binary with which and return the debian package it belongs to | ||
| command_to_package () { | ||
| local binary_path=$(readlink -f $(which $1)) | ||
| local binary_path | ||
| binary_path=$(readlink -f "$(which "$1")") |
There was a problem hiding this comment.
yeah, I don't like this new style, unless you also go and move all of the local definitions to the top of the function, c-style.
There was a problem hiding this comment.
yes, if you don't do assignment from subshells separately from local / export it masks the exit code https://github.com/koalaman/shellcheck/wiki/SC2155
There was a problem hiding this comment.
I'm not a fan of this style either, but the shellcheck lint seems convincing. I'm not sure the c style would be better, especially since this is only relevant to assignment which uses subshells, not literals / string formation
| echo "build --remote_http_cache=${cache_url}" | ||
| # specifically for bazel 0.15.0 we want to set this flag |
There was a problem hiding this comment.
do we think a fix might be cherry-picked for 0.15.1? or should we do this for all bazel 0.15 patch releases?
There was a problem hiding this comment.
this is a temporary check, we expect a fix to be in 0.15.1 if the fix is confirmed. I thought this would be good enough for now and avoid the question of handling semver. The goal of this PR is to make it possible to start testing this fix using a canary job and a :latest-experimental image
There was a problem hiding this comment.
we probably could match semver in bash and match a specific range with some regex though :^)
| # specifically for bazel 0.15.0 we want to set this flag | ||
| # our docker image now sets BAZEL_VERSION with the bazel version as installed | ||
| # https://github.com/bazelbuild/bazel/issues/5047#issuecomment-401295174 | ||
| if [[ -z "${BAZEL_VERSION+x}" && "${BAZEL_VERSION}" -eq "0.15.0" ]]; then |
There was a problem hiding this comment.
shouldn't the first check be -n "${BAZEL_VERSION+}"?
right now I think it'll always be false.
There was a problem hiding this comment.
though I don't think you need it at all, really. just do something like
if [[ "${BAZEL_VERSION:-}" -eq "0.15.0" ]]; then
...There was a problem hiding this comment.
yeah, shellcheck can't fix my misuse of -z / -n / -d :/
fixing
There was a problem hiding this comment.
so actually this was right but your suggestion is better. "${BAZEL_VERSION+x}" is zero if it evaluates to x and nonzero if bazel_version is actually set.
There was a problem hiding this comment.
what we really want is:
if [[ "${BAZEL_VERSION:-}" = "0.15.0" ]]; then
BenTheElder
force-pushed
the
bazel-experiments
branch
from
7947913 to
b314053
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, ixdy 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 |
|
I'll push a :latest-experimental image with 0.15.0 and start soak testing next |
BenTheElder
added
the
area/greenhouse
$BAZEL_VERSIONwith the version we're installing as an env instead of just an argcreate_bazel_cache_rcs.shso we can configure bazel 0.15.0 with a max concurrent connections for the remote cache (if this goes well the next patch release will have it as the default and we won't need to detect other versions)create_bazel_cache_rcs.shxref: bazelbuild/bazel#5047