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.

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

assorted small shell script fixes #113190

Merged
merged 8 commits into from Oct 20, 2022
15 changes: 10 additions & 5 deletions build/common.sh
Expand Up @@ -38,23 +38,28 @@ source "${KUBE_ROOT}/hack/lib/init.sh"

# Constants
readonly KUBE_BUILD_IMAGE_REPO=kube-build
readonly KUBE_BUILD_IMAGE_CROSS_TAG="$(cat "${KUBE_ROOT}/build/build-image/cross/VERSION")"
KUBE_BUILD_IMAGE_CROSS_TAG="$(cat "${KUBE_ROOT}/build/build-image/cross/VERSION")"
Copy link
Member Author

Choose a reason for hiding this comment

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

This file has a LOT of readonly FOO=....

We only need to alter those when they do complex assignments that may have a return value.

Lines like readonly KUBE_BUILD_IMAGE_REPO=kube-build we do not need to and it's not clearly worth the diff to change all of these, the linter should catch when it needs to be done (though, it clearly got smarter about this between v0.7.1 and v0.8.0).

readonly KUBE_BUILD_IMAGE_CROSS_TAG

readonly KUBE_DOCKER_REGISTRY="${KUBE_DOCKER_REGISTRY:-registry.k8s.io}"
readonly KUBE_BASE_IMAGE_REGISTRY="${KUBE_BASE_IMAGE_REGISTRY:-registry.k8s.io/build-image}"
KUBE_BASE_IMAGE_REGISTRY="${KUBE_BASE_IMAGE_REGISTRY:-registry.k8s.io/build-image}"
readonly KUBE_BASE_IMAGE_REGISTRY

# This version number is used to cause everyone to rebuild their data containers
# and build image. This is especially useful for automated build systems like
# Jenkins.
#
# Increment/change this number if you change the build image (anything under
# build/build-image) or change the set of volumes in the data container.
readonly KUBE_BUILD_IMAGE_VERSION_BASE="$(cat "${KUBE_ROOT}/build/build-image/VERSION")"
KUBE_BUILD_IMAGE_VERSION_BASE="$(cat "${KUBE_ROOT}/build/build-image/VERSION")"
readonly KUBE_BUILD_IMAGE_VERSION_BASE
readonly KUBE_BUILD_IMAGE_VERSION="${KUBE_BUILD_IMAGE_VERSION_BASE}-${KUBE_BUILD_IMAGE_CROSS_TAG}"

# Make it possible to override the `kube-cross` image, and tag independent of `KUBE_BASE_IMAGE_REGISTRY`
readonly KUBE_CROSS_IMAGE="${KUBE_CROSS_IMAGE:-"${KUBE_BASE_IMAGE_REGISTRY}/kube-cross"}"
readonly KUBE_CROSS_VERSION="${KUBE_CROSS_VERSION:-"${KUBE_BUILD_IMAGE_CROSS_TAG}"}"
KUBE_CROSS_IMAGE="${KUBE_CROSS_IMAGE:-"${KUBE_BASE_IMAGE_REGISTRY}/kube-cross"}"
readonly KUBE_CROSS_IMAGE
KUBE_CROSS_VERSION="${KUBE_CROSS_VERSION:-"${KUBE_BUILD_IMAGE_CROSS_TAG}"}"
readonly KUBE_CROSS_VERSION

# Here we map the output directories across both the local and remote _output
# directories:
Expand Down
2 changes: 1 addition & 1 deletion cluster/gce/util.sh
Expand Up @@ -2631,7 +2631,7 @@ function delete-subnetworks() {
# This value should be kept in sync with number of regions.
local parallelism=9
gcloud compute networks subnets list --network="${NETWORK}" --project "${NETWORK_PROJECT}" --format='value(region.basename())' | \
xargs -i -P ${parallelism} gcloud --quiet compute networks subnets delete "${NETWORK}" --project "${NETWORK_PROJECT}" --region="{}" || true
xargs -I {} -P ${parallelism} gcloud --quiet compute networks subnets delete "${NETWORK}" --project "${NETWORK_PROJECT}" --region="{}" || true
Copy link
Member Author

Choose a reason for hiding this comment

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

This does the same thing, just in a POSIX form.
GNU deprecated the non-POSIX form. https://www.shellcheck.net/wiki/SC2267

elif [[ "${CREATE_CUSTOM_NETWORK:-}" == "true" ]]; then
echo "Deleting custom subnet..."
gcloud --quiet compute networks subnets delete "${SUBNETWORK}" --project "${NETWORK_PROJECT}" --region="${REGION}" || true
Expand Down
11 changes: 6 additions & 5 deletions hack/lib/test.sh
Expand Up @@ -18,11 +18,12 @@

# A set of helpers for tests

readonly reset=$(tput sgr0)
readonly bold=$(tput bold)
readonly black=$(tput setaf 0)
readonly red=$(tput setaf 1)
readonly green=$(tput setaf 2)
reset=$(tput sgr0)
bold=$(tput bold)
black=$(tput setaf 0)
red=$(tput setaf 1)
green=$(tput setaf 2)
readonly reset bold black red green

kube::test::clear_all() {
if kube::test::if_supports_resource "rc" ; then
Expand Down
8 changes: 3 additions & 5 deletions hack/lib/util.sh
Expand Up @@ -603,11 +603,9 @@ function kube::util::list_staging_repos() {

# Determines if docker can be run, failures may simply require that the user be added to the docker group.
function kube::util::ensure_docker_daemon_connectivity {
IFS=" " read -ra DOCKER <<< "${DOCKER_OPTS}"
# Expand ${DOCKER[@]} only if it's not unset. This is to work around
# Bash 3 issue with unbound variable.
DOCKER=(docker ${DOCKER[@]:+"${DOCKER[@]}"})
if ! "${DOCKER[@]}" info > /dev/null 2>&1 ; then
DOCKER_OPTS=${DOCKER_OPTS:-""}
IFS=" " read -ra docker_opts <<< "${DOCKER_OPTS}"
if ! docker "${docker_opts[@]:+"${docker_opts[@]}"}" info > /dev/null 2>&1 ; then
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be functionally equivalent, without assigning DOCKER an array of docker + DOCKER_OPTS, potentially clobbering the ability to set DOCKER in other places where we intend it to be overridable.

The previous version was a bit strange and shellcheck was understandably not happy about mixing this as a string or an array in scripts that source util.sh.

cat <<'EOF' >&2
Can't connect to 'docker' daemon. please fix and retry.

Expand Down
5 changes: 3 additions & 2 deletions hack/make-rules/make-help.sh
Expand Up @@ -18,8 +18,9 @@ set -o errexit
set -o nounset
set -o pipefail

readonly red=$(tput setaf 1)
readonly reset=$(tput sgr0)
red=$(tput setaf 1)
reset=$(tput sgr0)
readonly red reset

KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
ALL_TARGETS=$(make -C "${KUBE_ROOT}" PRINT_HELP=y -rpn | sed -n -e '/^$/ { n ; /^[^ .#][^ ]*:/ { s/:.*$// ; p ; } ; }' | sort)
Expand Down
3 changes: 2 additions & 1 deletion hack/verify-e2e-test-ownership.sh
Expand Up @@ -41,7 +41,8 @@ fi
pushd "${KUBE_ROOT}" > /dev/null

# Setup a tmpdir to hold generated scripts and results
readonly tmpdir=$(mktemp -d -t verify-e2e-test-ownership.XXXX)
tmpdir=$(mktemp -d -t verify-e2e-test-ownership.XXXX)
readonly tmpdir
trap 'rm -rf ${tmpdir}' EXIT

# input
Expand Down
2 changes: 1 addition & 1 deletion hack/verify-generated-swagger-docs.sh
Expand Up @@ -70,7 +70,7 @@ ret=0
pushd "${KUBE_ROOT}" > /dev/null 2>&1
# Test for diffs
_output=""
for file in ${TARGET_FILES[*]}; do
for file in "${TARGET_FILES[@]}"; do
_output="${_output}$(diff -Naupr -I 'Auto generated by' "${KUBE_ROOT}/${file}" "${_kubetmp}/${file}")" || ret=1
done

Expand Down
4 changes: 4 additions & 0 deletions hack/verify-shellcheck.sh
Expand Up @@ -39,6 +39,10 @@ disabled=(
# this lint disallows non-constant source, which we use extensively without
# any known bugs
1090
# this lint warns when shellcheck cannot find a sourced file
# this wouldn't be a bad idea to warn on, but it fails on lots of path
# dependent sourcing, so just disable enforcing it
1091
BenTheElder marked this conversation as resolved.
Show resolved Hide resolved
# this lint prefers command -v to which, they are not the same
2230
)
Expand Down