Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix shellcheck failures in /hack/make-rules/update.sh,verify.sh #76420

Merged
merged 1 commit into from
May 16, 2019
Merged

fix shellcheck failures in /hack/make-rules/update.sh,verify.sh #76420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions hack/.shellcheck_failures
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
./hack/lib/version.sh
./hack/make-rules/make-help.sh
./hack/make-rules/test.sh
./hack/make-rules/update.sh
./hack/make-rules/verify.sh
./hack/pin-dependency.sh
./hack/test-integration.sh
./hack/update-vendor.sh
Expand Down
12 changes: 4 additions & 8 deletions hack/make-rules/update.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ set -o errexit
set -o nounset
set -o pipefail

KUBE_ROOT=$(dirname "${BASH_SOURCE}")/../..
KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
source "${KUBE_ROOT}/hack/lib/init.sh"

# If called directly, exit.
Expand All @@ -32,10 +32,6 @@ fi

SILENT=${SILENT:-true}
ALL=${FORCE_ALL:-false}
V=""
if [[ "${SILENT}" != "true" ]]; then
V="-v"
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/update.sh line 37:
        V="-v"★
        ^-- SC2034: V appears unused. Verify use (or export if used externally).

In #74877, godep-restore.sh" ${V} was deleted and I could not find scripts which use "V". So I deleted these lines.

trap 'exit 1' SIGINT

Expand All @@ -59,10 +55,10 @@ BASH_TARGETS="
update-gofmt"

for t in ${BASH_TARGETS}; do
echo -e "${color_yellow}Running ${t}${color_norm}"
echo -e "${color_yellow:?}Running ${t}${color_norm:?}"
if ${SILENT} ; then
if ! bash "${KUBE_ROOT}/hack/${t}.sh" 1> /dev/null; then
echo -e "${color_red}Running ${t} FAILED${color_norm}"
echo -e "${color_red:?}Running ${t} FAILED${color_norm}"
if ! ${ALL}; then
exit 1
fi
Expand All @@ -77,4 +73,4 @@ for t in ${BASH_TARGETS}; do
fi
done

echo -e "${color_green}Update scripts completed successfully${color_norm}"
echo -e "${color_green:?}Update scripts completed successfully${color_norm}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/update.sh line 61:
        echo -e "${color_yellow}Running ${t}${color_norm}"
                 ^-------------^ SC2154: color_yellow is referenced but not assigned.
                                            ^-----------^ SC2154: color_norm is referenced but not assigned.

In ./hack/make-rules/update.sh line 64:
                        echo -e "${color_red}Running ${t} FAILED${color_norm}"
                                 ^----------^ SC2154: color_red is referenced but not assigned.

In ./hack/make-rules/update.sh line 79:
echo -e "${color_green}Update scripts completed successfully${color_norm}"
         ^------------^ SC2154: color_green is referenced but not assigned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be coming in from lib/util.sh, I would rather we understand why that's not getting sourced in such that these are visible, rather than defend against them being unassigned

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spiffxp Thank you so much for the review and comments.
Is it better to add the following comment like #73746 than this modification?

for example

# shellcheck disable=SC2154 # color_green defined in lib/util.sh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is shellcheck not seeing this variable come in from that script, is this just not something it's capable of seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that shellcheck does not detect such variables and says warning from the following document.

According to the document, I think that ${var:?} or disabling is reasonable change.
Should we consider any other changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This message only triggers for variables with lowercase characters in their name (foo and kFOO but not FOO) due to the standard convention of using lowercase variable names for unexported, local variables.

Is the explanation I was looking for. OK, yeah, ${var:?} seems valid here then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other "right" way to address this would be to insist all of the color_foo vars be upcased, but I'm not going to insist on that as it will blow scope here and likely fall prey to merge conflicts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I see.

32 changes: 17 additions & 15 deletions hack/make-rules/verify.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ set -o errexit
set -o nounset
set -o pipefail

KUBE_ROOT=$(dirname "${BASH_SOURCE}")/../..
KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
source "${KUBE_ROOT}/hack/lib/util.sh"

# If KUBE_JUNIT_REPORT_DIR is unset, and ARTIFACTS is set, then have them match.
Expand Down Expand Up @@ -80,13 +80,13 @@ QUICK_PATTERNS+=(
"verify-test-owners.sh"
)

EXCLUDED_CHECKS=$(ls ${EXCLUDED_PATTERNS[@]/#/${KUBE_ROOT}\/hack\/} 2>/dev/null || true)
QUICK_CHECKS=$(ls ${QUICK_PATTERNS[@]/#/${KUBE_ROOT}\/hack\/} 2>/dev/null || true)
while IFS='' read -r line; do EXCLUDED_CHECKS+=("$line"); done < <(ls "${EXCLUDED_PATTERNS[@]/#/${KUBE_ROOT}\/hack\/}" 2>/dev/null || true)
while IFS='' read -r line; do QUICK_CHECKS+=("$line"); done < <(ls "${QUICK_PATTERNS[@]/#/${KUBE_ROOT}\/hack\/}" 2>/dev/null || true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/verify.sh line 84:
EXCLUDED_CHECKS=$(ls ${EXCLUDED_PATTERNS[@]/#/${KUBE_ROOT}\/hack\/} 2>/dev/null || true)
                     ^-- SC2068: Double quote array expansions to avoid re-splitting elements.

In ./hack/make-rules/verify.sh line 85:
QUICK_CHECKS=$(ls ${QUICK_PATTERNS[@]/#/${KUBE_ROOT}\/hack\/} 2>/dev/null || true)
                  ^-- SC2068: Double quote array expansions to avoid re-splitting elements.

When adding only double quotation, EXCLUDED_CHECKS/QUICK_CHECKS are handled as string not array.
So, is-excluded/is-quick functions cannot work properly.
Therefore, I changed these lines as the above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmed this works locally

TARGET_LIST=()
IFS=" " read -r -a TARGET_LIST <<< "${WHAT:-}"

function is-excluded {
for e in ${EXCLUDED_CHECKS[@]}; do
for e in "${EXCLUDED_CHECKS[@]}"; do
if [[ $1 -ef "${e}" ]]; then
return
fi
Expand All @@ -95,7 +95,7 @@ function is-excluded {
}

function is-quick {
for e in ${QUICK_CHECKS[@]}; do
for e in "${QUICK_CHECKS[@]}"; do
if [[ $1 -ef "${e}" ]]; then
return
fi
Expand Down Expand Up @@ -138,9 +138,9 @@ FAILED_TESTS=()

function print-failed-tests {
echo -e "========================"
echo -e "${color_red}FAILED TESTS${color_norm}"
echo -e "${color_red:?}FAILED TESTS${color_norm:?}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be coming in from lib/util.sh, I would rather we understand why that's not getting sourced in such that these are visible, rather than defend against them being unassigned

echo -e "========================"
for t in ${FAILED_TESTS[@]}; do
for t in "${FAILED_TESTS[@]}"; do
echo -e "${color_red}${t}${color_norm}"
done
}
Expand All @@ -150,10 +150,11 @@ function run-checks {
local -r runner=$2

local t
for t in $(ls ${pattern})
for t in ${pattern}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/verify.sh line 149:
  for t in $(ls ${pattern})
           ^--------------^ SC2045: Iterating over ls output is fragile. Use globs.
                ^--------^ SC2086: Double quote to prevent globbing and word splitting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rename pattern to glob so it's clearer that's the intent here?

do
local check_name="$(basename "${t}")"
if [[ ! -z ${WHAT:-} ]]; then
local check_name
check_name="$(basename "${t}")"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/verify.sh line 151:
    local check_name="$(basename "${t}")"
          ^--------^ SC2155: Declare and assign separately to avoid masking return values.

if [[ -n ${WHAT:-} ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/verify.sh line 152:
    if [[ ! -z ${WHAT:-} ]]; then
          ^-- SC2236: Use -n instead of ! -z.

if ! is-explicitly-chosen "${check_name}"; then
continue
fi
Expand All @@ -168,15 +169,16 @@ function run-checks {
fi
fi
echo -e "Verifying ${check_name}"
local start=$(date +%s)
local start
start=$(date +%s)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/verify.sh line 167:
    local start=$(date +%s)
          ^---^ SC2155: Declare and assign separately to avoid masking return values.

run-cmd "${runner}" "${t}" && tr=$? || tr=$?
local elapsed=$(($(date +%s) - ${start}))
local elapsed=$(($(date +%s) - start))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In ./hack/make-rules/verify.sh line 169:
    local elapsed=$(($(date +%s) - ${start}))
                                   ^------^ SC2004: $/${} is unnecessary on arithmetic variables.

if [[ ${tr} -eq 0 ]]; then
echo -e "${color_green}SUCCESS${color_norm} ${check_name}\t${elapsed}s"
echo -e "${color_green:?}SUCCESS${color_norm} ${check_name}\t${elapsed}s"
else
echo -e "${color_red}FAILED${color_norm} ${check_name}\t${elapsed}s"
ret=1
FAILED_TESTS+=(${t})
FAILED_TESTS+=("${t}")
fi
done
}
Expand All @@ -190,7 +192,7 @@ function missing-target-checks {
do
[[ -z "${v}" ]] && continue

FAILED_TESTS+=(${v})
FAILED_TESTS+=("${v}")
ret=1
done
}
Expand Down