Juju kube up #23440
Juju kube up #23440
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
2 similar comments
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
Labelling this PR as size/XS |
k8s-github-robot
added
kind/old-docs
size/XS
|
@mbruzek Hey thanks for the cleanup in the kube-up script. The auto fetch is a welcome addition 👍 LGTM |
| @@ -30,6 +30,8 @@ source "${JUJU_PATH}/${KUBE_CONFIG_FILE-config-default.sh}" | |||
| source ${JUJU_PATH}/prereqs/ubuntu-juju.sh | |||
| export JUJU_REPOSITORY=${JUJU_PATH}/charms | |||
| KUBE_BUNDLE_PATH=${JUJU_PATH}/bundles/local.yaml | |||
| # The destination directory for the kubectl binary file. | |||
| KUBECTL_DIR=${KUBE_ROOT}/platforms/linux/amd64 | |||
There was a problem hiding this comment.
must be quoted (this whole file does a terrible job quoting bash variable, so lets start making them better)
KUBECTL_DIR="${KUBE_ROOT}/platforms/linux/amd64
This applies to every line you touch. I won't comment on every single line :)
|
So I don't get it. All you did was move the destination directory and change the KUBECONFIG to point to this new destination dir. What was wrong with the old destination directory? How does this fix something? Is there some random by chance side effect that code is looking at KUBEROOT/platform/... which is just waiting to be broken in the future? Is KUBECONFIG inadequate? |
|
@eparis Thanks for the review! The reason for this PR is to get the Yes the path is hardcoded, because the I will make the changes to quote the strings and ping you when that is done. Thanks again for the review! |
|
So rather than having it find it by random side effect, why didn't you set KUBECTL_PATH ? Or put it in the $PATH ? |
|
to be fair, I'm fine with the change in location AND setting KUBECTL_PATH.... |
|
ok to test |
eparis
added
e2e-not-required
release-note-none
|
please squash to one commit. |
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
I think your squash went badly..... |
|
@eparis working on it. |
|
CLAs look good, thanks! |
eparis
added
the
lgtm
|
that looks much happier :) |
|
Labelling this PR as size/S |
k8s-github-robot
added
size/S
|
GCE e2e build/test passed for commit 2c8736d2d9dbd8b99a6520983a8e28ff2889e83b. |
|
GCE e2e build/test passed for commit 03002a16dc6dbe0f211a00cfe0cb595922fe45bb. |
|
GCE e2e build/test passed for commit 64d849e. |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
Automatic merge from submit-queue |
I found some problems with the kube-up script that this pull request addresses. We didn't have the kubectl binary in the correct location.
Just changing where we download the package from the master, and fixing the kube-down.sh script to remove those files.