From 21431bb4b0adb5fbe8234b36a84ee89b30dcd773 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 05:34:25 -0800 Subject: [PATCH 1/8] scope launch script: Fix shellcheck linter warnings By far the majority of these were variables which were not quoted. While, yes, right now we can guarentee most of these variables will never contain spaces, this could someday change and applying quoting as a universal rule prevents future mistakes. The ARGS="$@" -> "$*" change is purely stylistic and mainly is used to indicate the intent that we actually wanted to concatenate all the args by spaces, not keep them seperated as "$@" would in many situations, but not this one. Several warnings remain, in places where we intentionally want to split a variable on whitespace, or otherwise do what shellcheck is warning us against. Of note is shellcheck warning SC2166, which says to prefer [ foo ] || [ bar ] over [ foo -o bar ] as the -a and -o flags have differing behaviour on some systems. I've opted to keep these for now, since the version check test command would need to be replaced by a LOT of subshells to achieve the same effect, which feels dirtier. --- scope | 56 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/scope b/scope index 4fdb07786..9bb0ddbed 100755 --- a/scope +++ b/scope @@ -2,7 +2,7 @@ set -eu -ARGS="$@" +ARGS="$*" usage() { echo "Usage:" @@ -18,11 +18,11 @@ SCRIPT_VERSION="(unreleased version)" if [ "$SCRIPT_VERSION" = "(unreleased version)" ] ; then IMAGE_VERSION=latest else - IMAGE_VERSION=$SCRIPT_VERSION + IMAGE_VERSION="$SCRIPT_VERSION" fi IMAGE_VERSION=${VERSION:-$IMAGE_VERSION} SCOPE_IMAGE_NAME=weaveworks/scope -SCOPE_IMAGE=$SCOPE_IMAGE_NAME:$IMAGE_VERSION +SCOPE_IMAGE="$SCOPE_IMAGE_NAME:$IMAGE_VERSION" SCOPE_CONTAINER_NAME=weavescope SCOPE_APP_CONTAINER_NAME=weavescope-app IP_REGEXP="[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}" @@ -98,7 +98,7 @@ check_docker_for_mac() { # Check that a container named $1 with image $2 is not running check_not_running() { - case $(docker inspect --format='{{.State.Running}} {{.Config.Image}}' $1 2>/dev/null) in + case $(docker inspect --format='{{.State.Running}} {{.Config.Image}}' "$1" 2>/dev/null) in "true $2") echo "$1 is already running." >&2 exit 1 @@ -108,10 +108,10 @@ check_not_running() { exit 1 ;; "false $2") - docker rm $1 >/dev/null + docker rm "$1" >/dev/null ;; "false $2:"*) - docker rm $1 >/dev/null + docker rm "$1" >/dev/null ;; true*) echo "Found another running container named '$1'. Aborting." >&2 @@ -135,29 +135,29 @@ create_plugins_dir() { # sockets do not cross VM boundaries. We need this directory to exits on the VM. docker run --rm --entrypoint=/bin/sh \ -v /var/run:/var/run \ - $SCOPE_IMAGE -c "mkdir -p /var/run/scope/plugins" + "$SCOPE_IMAGE" -c "mkdir -p /var/run/scope/plugins" } launch_command() { - echo docker run --privileged -d --name=$SCOPE_CONTAINER_NAME --net=host --pid=host \ + echo docker run --privileged -d --name="$SCOPE_CONTAINER_NAME" --net=host --pid=host \ -v /var/run/docker.sock:/var/run/docker.sock \ -v /var/run/scope/plugins:/var/run/scope/plugins \ -e CHECKPOINT_DISABLE \ - $WEAVESCOPE_DOCKER_ARGS $SCOPE_IMAGE --probe.docker=true + $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" --probe.docker=true } launch_docker4mac_app_command() { - echo docker run -d --name=$SCOPE_APP_CONTAINER_NAME \ + echo docker run -d --name="$SCOPE_APP_CONTAINER_NAME" \ -e CHECKPOINT_DISABLE \ -p 0.0.0.0:4040:4040 \ - $WEAVESCOPE_DOCKER_ARGS $SCOPE_IMAGE --no-probe + $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" --no-probe } launch() { - check_not_running $SCOPE_CONTAINER_NAME $SCOPE_IMAGE_NAME - docker rm -f $SCOPE_CONTAINER_NAME >/dev/null 2>&1 || true + check_not_running "$SCOPE_CONTAINER_NAME" "$SCOPE_IMAGE_NAME" + docker rm -f "$SCOPE_CONTAINER_NAME" >/dev/null 2>&1 || true CONTAINER=$($(launch_command) "$@") - echo $CONTAINER + echo "$CONTAINER" } print_app_endpoints() { @@ -173,12 +173,12 @@ check_docker_version case "$COMMAND" in command) # TODO: properly escape/quote the output of "$@" - echo $(launch_command) "$@" + echo "$(launch_command)" "$@" ;; version) docker run --rm -e CHECKPOINT_DISABLE --entrypoint=/home/weave/scope \ - $WEAVESCOPE_DOCKER_ARGS $SCOPE_IMAGE --mode=version + $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" --mode=version ;; help) @@ -190,7 +190,7 @@ scope help - Print this scope launch - Launch Scope EOF docker run --rm -e CHECKPOINT_DISABLE --entrypoint=/home/weave/scope \ - $WEAVESCOPE_DOCKER_ARGS $SCOPE_IMAGE -h + $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" -h cat >&2 </dev/null 2>&1 || true + check_not_running "$SCOPE_APP_CONTAINER_NAME" "$SCOPE_IMAGE_NAME" + check_not_running "$SCOPE_CONTAINER_NAME" "$SCOPE_IMAGE_NAME" + docker rm -f "$SCOPE_APP_CONTAINER_NAME" >/dev/null 2>&1 || true CONTAINER=$($(launch_docker4mac_app_command) "$@") - echo $CONTAINER + echo "$CONTAINER" app_ip=$(docker inspect -f '{{.NetworkSettings.IPAddress}}' "${CONTAINER}") - docker rm -f $SCOPE_CONTAINER_NAME >/dev/null 2>&1 || true + docker rm -f "$SCOPE_CONTAINER_NAME" >/dev/null 2>&1 || true CONTAINER=$($(launch_command --no-app "$@" "${app_ip}:4040")) print_app_endpoints "localhost" exit @@ -232,7 +232,7 @@ EOF launch "$@" if ! check_probe_only ; then - IP_ADDRS=$(docker run --rm --net=host --entrypoint /bin/sh $SCOPE_IMAGE -c "$IP_ADDR_CMD") + IP_ADDRS=$(docker run --rm --net=host --entrypoint /bin/sh "$SCOPE_IMAGE" -c "$IP_ADDR_CMD") print_app_endpoints $IP_ADDRS fi @@ -240,12 +240,12 @@ EOF stop) [ $# -eq 0 ] || usage - if docker inspect $SCOPE_CONTAINER_NAME >/dev/null 2>&1 ; then - docker stop $SCOPE_CONTAINER_NAME >/dev/null + if docker inspect "$SCOPE_CONTAINER_NAME" >/dev/null 2>&1 ; then + docker stop "$SCOPE_CONTAINER_NAME" >/dev/null fi if check_docker_for_mac ; then - if docker inspect $SCOPE_APP_CONTAINER_NAME >/dev/null 2>&1 ; then - docker stop $SCOPE_APP_CONTAINER_NAME >/dev/null + if docker inspect "$SCOPE_APP_CONTAINER_NAME" >/dev/null 2>&1 ; then + docker stop "$SCOPE_APP_CONTAINER_NAME" >/dev/null fi fi ;; From b1a1c7d427bccc1df4723282fdd7a775ebc7b13f Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 05:57:28 -0800 Subject: [PATCH 2/8] scope launch script: Attempt to quote args in "scope command" As indicated by the TODO, any args passed into the command do not get escaped when output, so for example: scope command "foo bar" would output results like: foo bar instead of "foo bar" or foo\ bar The "right" way to do this seems to be printf %q, which prints a quoted version of the string. However this format specifier is not available in POSIX sh (though it does work in many implementations of it, such as the ones provided by bash which make up the likely majority of real-world usage). This code is a compromise that uses the added functionality where available, while still falling back to the old behaviour when it isn't. --- scope | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/scope b/scope index 9bb0ddbed..010411c8b 100755 --- a/scope +++ b/scope @@ -172,8 +172,16 @@ check_docker_version case "$COMMAND" in command) - # TODO: properly escape/quote the output of "$@" - echo "$(launch_command)" "$@" + # Most systems should have printf, but the %q specifier isn't mandated by posix + # and can't be guaranteed. Since this is mainly a cosmetic output and the alternative + # is not making any attempt to do escaping at all, we might as well try. + quoted=$(printf '%q ' "$@" 2>/dev/null || true) + # printf %q behaves oddly with zero args (it acts as though it recieved one empty arg) + # so we ignore that case. + if [ -z "$quoted" ] || [ $# -eq 0 ]; then + quoted="$*" + fi + echo "$(launch_command) $quoted" ;; version) @@ -245,7 +253,7 @@ EOF fi if check_docker_for_mac ; then if docker inspect "$SCOPE_APP_CONTAINER_NAME" >/dev/null 2>&1 ; then - docker stop "$SCOPE_APP_CONTAINER_NAME" >/dev/null + docker stop "$SCOPE_APP_CONTAINER_NAME" >/dev/null fi fi ;; From 90709b61aa892bb83791caa922fc869b877b09c7 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 07:11:59 -0800 Subject: [PATCH 3/8] scope launch script: Fix and consolidate usage info Instead of different usage info for "scope help", show the same always. Also correct it for what the script actually does, and always display the scope binary args. --- scope | 57 ++++++++++++++++++++++++++------------------------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/scope b/scope index 010411c8b..7cc5ce23c 100755 --- a/scope +++ b/scope @@ -3,17 +3,6 @@ set -eu ARGS="$*" - -usage() { - echo "Usage:" - echo "scope launch [ ...]" - echo "scope stop" - echo "scope command" - echo - echo "scope is of the form [:]" - exit 1 -} - SCRIPT_VERSION="(unreleased version)" if [ "$SCRIPT_VERSION" = "(unreleased version)" ] ; then IMAGE_VERSION=latest @@ -30,7 +19,29 @@ IP_ADDR_CMD="find /sys/class/net -type l | xargs -n1 basename | grep -vE 'docker xargs -n1 ip addr show | grep inet | awk '{ print \$2 }' | grep -oE '$IP_REGEXP'" WEAVESCOPE_DOCKER_ARGS=${WEAVESCOPE_DOCKER_ARGS:-} -[ $# -gt 0 ] || usage +usage() { + name=$(basename "$0") + cat >&2 <<-EOF + Usage: + $name launch {ARGS} - Launch Scope + $name stop - Stop Scope + $name command - Print the docker command used to start Scope + $name help - Print usage info + $name version - Print version info + + Launch arguments: + EOF + docker run --rm -e CHECKPOINT_DISABLE --entrypoint=/home/weave/scope \ + $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" -h >&2 || + echo "Error listing launch arguments" >&2 +} + +usage_and_die() { + usage + exit 1 +} + +[ $# -gt 0 ] || usage_and_die COMMAND=$1 shift 1 @@ -190,23 +201,7 @@ case "$COMMAND" in ;; help) - cat >&2 <&2 </dev/null 2>&1 ; then docker stop "$SCOPE_CONTAINER_NAME" >/dev/null fi @@ -260,7 +255,7 @@ EOF *) echo "Unknown scope command '$COMMAND'" >&2 - usage + usage_and_die ;; esac From 3a69c19d6e41e5474f31bf0ccc31b1f76a9b5641 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 07:20:15 -0800 Subject: [PATCH 4/8] scope launch script: Also recongize -h, -help and --help --- scope | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scope b/scope index 7cc5ce23c..b0d4a4687 100755 --- a/scope +++ b/scope @@ -200,7 +200,7 @@ case "$COMMAND" in $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" --mode=version ;; - help) + -h|help|-help|--help) usage ;; From 0e4e95e4fa786a788c264015a6e6e7e417f10389 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 08:03:18 -0800 Subject: [PATCH 5/8] scope launch script: Edit help text to make positional args clearer --- scope | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/scope b/scope index b0d4a4687..4fb1b9a2e 100755 --- a/scope +++ b/scope @@ -23,13 +23,14 @@ usage() { name=$(basename "$0") cat >&2 <<-EOF Usage: - $name launch {ARGS} - Launch Scope - $name stop - Stop Scope - $name command - Print the docker command used to start Scope - $name help - Print usage info - $name version - Print version info + $name launch {OPTIONS} {PEERS} - Launch Scope + $name stop - Stop Scope + $name command - Print the docker command used to start Scope + $name help - Print usage info + $name version - Print version info - Launch arguments: + PEERS are of the form HOST[:PORT], and HOST may be an ip or hostname. + Launch options: EOF docker run --rm -e CHECKPOINT_DISABLE --entrypoint=/home/weave/scope \ $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" -h >&2 || From 7333b99b89ee76a091dd83d24cb31e8b5556042b Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 08:05:44 -0800 Subject: [PATCH 6/8] scope launch script: Make the default port clear --- scope | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scope b/scope index 4fb1b9a2e..7fa8f875b 100755 --- a/scope +++ b/scope @@ -29,7 +29,10 @@ usage() { $name help - Print usage info $name version - Print version info - PEERS are of the form HOST[:PORT], and HOST may be an ip or hostname. + PEERS are of the form HOST[:PORT] + HOST may be an ip or hostname. + PORT defaults to 4040. + Launch options: EOF docker run --rm -e CHECKPOINT_DISABLE --entrypoint=/home/weave/scope \ From 3000adfb9a76974e327e65e986edd82447526859 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 08:08:25 -0800 Subject: [PATCH 7/8] scope launch script: Don't print container id It's not useful and it's confusing. Replaced with a generic "it worked" message. --- scope | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scope b/scope index 7fa8f875b..c2924e696 100755 --- a/scope +++ b/scope @@ -171,8 +171,8 @@ launch_docker4mac_app_command() { launch() { check_not_running "$SCOPE_CONTAINER_NAME" "$SCOPE_IMAGE_NAME" docker rm -f "$SCOPE_CONTAINER_NAME" >/dev/null 2>&1 || true - CONTAINER=$($(launch_command) "$@") - echo "$CONTAINER" + $(launch_command) "$@" + echo "Scope probe started" } print_app_endpoints() { @@ -228,8 +228,8 @@ case "$COMMAND" in check_not_running "$SCOPE_APP_CONTAINER_NAME" "$SCOPE_IMAGE_NAME" check_not_running "$SCOPE_CONTAINER_NAME" "$SCOPE_IMAGE_NAME" docker rm -f "$SCOPE_APP_CONTAINER_NAME" >/dev/null 2>&1 || true - CONTAINER=$($(launch_docker4mac_app_command) "$@") - echo "$CONTAINER" + $(launch_docker4mac_app_command) "$@" + echo "Scope probe started" app_ip=$(docker inspect -f '{{.NetworkSettings.IPAddress}}' "${CONTAINER}") docker rm -f "$SCOPE_CONTAINER_NAME" >/dev/null 2>&1 || true CONTAINER=$($(launch_command --no-app "$@" "${app_ip}:4040")) From 2562567de4aa6d1780d4e1f2310f24c05ade9567 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 9 Dec 2016 11:34:14 -0800 Subject: [PATCH 8/8] scope launch script: Don't try to detect if listing scope binary args worked We can't easily do this since scope exits failure when -h is passed, so we can't distinugish between success and failure. --- scope | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scope b/scope index c2924e696..36b11a213 100755 --- a/scope +++ b/scope @@ -36,8 +36,7 @@ usage() { Launch options: EOF docker run --rm -e CHECKPOINT_DISABLE --entrypoint=/home/weave/scope \ - $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" -h >&2 || - echo "Error listing launch arguments" >&2 + $WEAVESCOPE_DOCKER_ARGS "$SCOPE_IMAGE" -h >&2 } usage_and_die() {