Don't assume that gcloud is in the same place as gsutil. #695
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
|
/cc mtaufen |
lib/common.sh
Outdated
| # gcloud should be in the same place | ||
| GCLOUD=${GSUTIL/gsutil/gcloud} | ||
| for GCLOUD in "$GSUTIL/gsutil/gcloud" "$(which gcloud)"; do | ||
| [[ -x $GCLOUD ]] && break |
There was a problem hiding this comment.
Can you make this an if statement instead of the && shorthand?
k8s-ci-robot
added
size/S
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dougm, mtaufen, pjh 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
|
Multiple of us have this patched code working fine, but it seems like when it runs in GCloud it fails? @Bubblemelon and I both have a failure on what might be this change when we "gcbmbr stage master": |
|
Sorry for the breakage. I don't know why the script is failing to find gsutil and gcloud when run inside of the gcr.io/kubernetes-release-test/k8s-cloud-builder container. Tracking that down will either require rebuilding the container with some additional debug logging in the script, or exec'ing into the container and running the script manually with additional debug logging. cherylfong on Slack has graciously offered to take a quick look at this. If that fails then I've sent #698 to rollback this change. |
|
@pjh. Please revert this change. The only context that really maters here is when this tooling is executed within GCB, the particulars of an arbitrary development machine should not be considered here. |
|
The rollback has been merged: #698. |
| done | ||
|
|
||
| # gcloud should be in the same place | ||
| GCLOUD=${GSUTIL/gsutil/gcloud} | ||
| for GCLOUD in "${GSUTIL}/gsutil/gcloud" "$(which gcloud)"; do |
There was a problem hiding this comment.
This is different in meaning because in the original code, ${GSUTIL/gsutil/gcloud}is doing a pattern substitution (brace expansion). In your change you are treating the slashes as path delimiters in a filesystem. So the first value in the list won't be executable (it most likely doesn't exist). And in the Docker container the path to gcloud is probably not in the PATH, so the second command probably fails too ("which" will fail).
That's probably why it broke in the container, is my guess.
There was a problem hiding this comment.
${GSUTIL/gsutil/gcloud}is doing a pattern substitution (brace expansion)
Wow, yeah I totally overlooked that. My fault for changing it without understanding its effects, but I had no idea bash could do pattern substitution in this manner, and just assumed that was a path.
Thanks for the explanation.
|
Oops, looks like you already fixed this in #705. Sorry for the noise. |
gsutil and gcloud are not in the same directory on my system and I don't see why they should have to be. This PR searches for gcloud both in the gsutil directory as well as wherever in the user's PATH it might be.
I've tested that this works on my own system and I think it shouldn't break any existing uses but hopefully reviewers with more bash experience can confirm (or tell me how to test).