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 hack/lib/swagger.sh, hack/lib/init.sh, hack/lib/version.sh shellcheck failures #79320

Merged
merged 3 commits into from
Jun 28, 2019
Merged

fix hack/lib/swagger.sh, hack/lib/init.sh, hack/lib/version.sh shellcheck failures #79320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a101acc
fix hack/lib/swagger.sh shellcheck failures
BenTheElder Jun 24, 2019
9b4b509
fix hack/lib/init.sh shellcheck failures
BenTheElder Jun 24, 2019
59c4eb2
fix hack/lib/version.sh shellcheck failures
BenTheElder Jun 24, 2019
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
3 changes: 0 additions & 3 deletions hack/.shellcheck_failures
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@
./cluster/restore-from-backup.sh
./cluster/test-e2e.sh
./cluster/validate-cluster.sh
./hack/lib/init.sh
./hack/lib/swagger.sh
./hack/lib/test.sh
./hack/lib/version.sh
./hack/test-integration.sh
./hack/verify-test-featuregates.sh
./test/cmd/diff.sh
Expand Down
6 changes: 4 additions & 2 deletions hack/lib/init.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ unset CDPATH
export GO111MODULE=auto

# The root of the build/dist directory
KUBE_ROOT="$(cd "$(dirname "${BASH_SOURCE}")/../.." && pwd -P)"
KUBE_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd -P)"

KUBE_OUTPUT_SUBPATH="${KUBE_OUTPUT_SUBPATH:-_output/local}"
KUBE_OUTPUT="${KUBE_ROOT}/${KUBE_OUTPUT_SUBPATH}"
Expand All @@ -44,7 +44,7 @@ KUBE_RSYNC_COMPRESS="${KUBE_RSYNC_COMPRESS:-0}"
export no_proxy=127.0.0.1,localhost

# This is a symlink to binaries for "this platform", e.g. build tools.
THIS_PLATFORM_BIN="${KUBE_ROOT}/_output/bin"
export THIS_PLATFORM_BIN="${KUBE_ROOT}/_output/bin"
Copy link
Member Author

Choose a reason for hiding this comment

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

I confirmed that this is read (by golang.sh), the same applies for KUBE_OUTPUT_HOSTBIN, and KUBE_NONSERVER_GROUP_VERSIONS is read in multiple places


source "${KUBE_ROOT}/hack/lib/util.sh"
source "${KUBE_ROOT}/hack/lib/logging.sh"
Expand All @@ -56,6 +56,7 @@ source "${KUBE_ROOT}/hack/lib/golang.sh"
source "${KUBE_ROOT}/hack/lib/etcd.sh"

KUBE_OUTPUT_HOSTBIN="${KUBE_OUTPUT_BINPATH}/$(kube::util::host_platform)"
export KUBE_OUTPUT_HOSTBIN

# list of all available group versions. This should be used when generated code
# or when starting an API server that you want to have everything.
Expand Down Expand Up @@ -110,6 +111,7 @@ KUBE_NONSERVER_GROUP_VERSIONS="
imagepolicy.k8s.io/v1alpha1\
admission.k8s.io/v1beta1\
"
export KUBE_NONSERVER_GROUP_VERSIONS

# This emulates "readlink -f" which is not available on MacOS X.
# Test:
Expand Down
16 changes: 8 additions & 8 deletions hack/lib/swagger.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,22 @@ set -o errexit
set -o nounset
set -o pipefail

# The root of the build/dist directory
KUBE_ROOT="$(cd "$(dirname "${BASH_SOURCE}")/../.." && pwd -P)"

# Generates types_swagger_doc_generated file for the given group version.
# $1: Name of the group version
# $2: Path to the directory where types.go for that group version exists. This
# is the directory where the file will be generated.
kube::swagger::gen_types_swagger_doc() {
local group_version=$1
local gv_dir=$2
local TMPFILE="${TMPDIR:-/tmp}/types_swagger_doc_generated.$(date +%s).go"
local TMPFILE
TMPFILE="${TMPDIR:-/tmp}/types_swagger_doc_generated.$(date +%s).go"
Copy link
Member Author

Choose a reason for hiding this comment

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

non-trivial assignment should be separate from declaration in order to not swallow the error


echo "Generating swagger type docs for ${group_version} at ${gv_dir}"

echo -e "$(cat hack/boilerplate/boilerplate.generatego.txt)\n" > "${TMPFILE}"
echo "package ${group_version##*/}" >> "${TMPFILE}"
cat >> "${TMPFILE}" <<EOF
{
echo -e "$(cat hack/boilerplate/boilerplate.generatego.txt)\n"
echo "package ${group_version##*/}"
cat <<EOF

// This file contains a collection of methods that can be used from go-restful to
// generate Swagger API documentation for its models. Please read this PR for more
Expand All @@ -50,6 +49,7 @@ kube::swagger::gen_types_swagger_doc() {

// AUTO-GENERATED FUNCTIONS START HERE. DO NOT EDIT.
EOF
} > "${TMPFILE}"
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 is mostly equivalent but more efficient (only opens & redirects once) and shellcheck prefers grouping commands when you appear to be redirecting many commands to the same place

Copy link
Member Author

Choose a reason for hiding this comment

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

we could turn off the lint instead, but this is actually a minor improvement


go run cmd/genswaggertypedocs/swagger_type_docs.go -s \
"${gv_dir}/types.go" \
Expand All @@ -59,5 +59,5 @@ EOF
echo "// AUTO-GENERATED FUNCTIONS END HERE" >> "${TMPFILE}"

gofmt -w -s "${TMPFILE}"
mv "${TMPFILE}" ""${gv_dir}"/types_swagger_doc_generated.go"
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 quoting was totally wrong but if your path didn't have spaces it would work out

mv "${TMPFILE}" "${gv_dir}/types_swagger_doc_generated.go"
}
47 changes: 27 additions & 20 deletions hack/lib/version.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ kube::version::get_version_vars() {

# If the kubernetes source was exported through git archive, then
# we likely don't have a git tree, but these magic values may be filled in.
# shellcheck disable=SC2016,SC2050
# Disabled as we're not expanding these at runtime, but rather expecting
# that another tool may have expanded these and rewritten the source (!)
if [[ '$Format:%%$' == "%" ]]; then
KUBE_GIT_COMMIT='$Format:%H$'
KUBE_GIT_TREE_STATE="archive"
Expand Down Expand Up @@ -70,11 +73,17 @@ kube::version::get_version_vars() {
#
# TODO: We continue calling this "git version" because so many
# downstream consumers are expecting it there.
#
# These regexes are painful enough in sed...
# We don't want to do them in pure shell, so disable SC2001
# shellcheck disable=SC2001
DASHES_IN_VERSION=$(echo "${KUBE_GIT_VERSION}" | sed "s/[^-]//g")
if [[ "${DASHES_IN_VERSION}" == "---" ]] ; then
# shellcheck disable=SC2001
# We have distance to subversion (v1.1.0-subversion-1-gCommitHash)
KUBE_GIT_VERSION=$(echo "${KUBE_GIT_VERSION}" | sed "s/-\([0-9]\{1,\}\)-g\([0-9a-f]\{14\}\)$/.\1\+\2/")
Copy link
Member Author

Choose a reason for hiding this comment

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

in particular I really don't think we want to try to do this in pure shell parameter substitution (not even sure if you can?) ... using sed here is immensely reasonable, so we disable the lint against unnecessary sed usage.

the shellcheck wiki entry has a similar example.

elif [[ "${DASHES_IN_VERSION}" == "--" ]] ; then
# shellcheck disable=SC2001
# We have distance to base tag (v1.1.0-1-gCommitHash)
KUBE_GIT_VERSION=$(echo "${KUBE_GIT_VERSION}" | sed "s/-g\([0-9a-f]\{14\}\)$/+\1/")
fi
Expand Down Expand Up @@ -135,40 +144,38 @@ kube::version::load_version_vars() {
source "${version_file}"
}

kube::version::ldflag() {
Copy link
Member Author

Choose a reason for hiding this comment

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

because this generates 3 entries for an array and is called repeatedly for the same array it is difficult to correctly wrap it without splitting concerns etc.

instead i've nested it in the only caller (kube::version::ldflags) and have it append to the array directly. much simpler.

local key=${1}
local val=${2}

# If you update these, also update the list pkg/version/def.bzl.
echo "-X '${KUBE_GO_PACKAGE}/pkg/version.${key}=${val}'"
echo "-X '${KUBE_GO_PACKAGE}/vendor/k8s.io/client-go/pkg/version.${key}=${val}'"
echo "-X '${KUBE_GO_PACKAGE}/pkg/kubectl/version.${key}=${val}'"
}

# Prints the value that needs to be passed to the -ldflags parameter of go build
# in order to set the Kubernetes based on the git tree status.
# IMPORTANT: if you update any of these, also update the lists in
# pkg/version/def.bzl and hack/print-workspace-status.sh.
kube::version::ldflags() {
kube::version::get_version_vars

local buildDate=
[[ -z ${SOURCE_DATE_EPOCH-} ]] || buildDate="--date=@${SOURCE_DATE_EPOCH}"
Copy link
Member Author

Choose a reason for hiding this comment

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

all of this logic amounts to either expanding to a --date=@${SOURCE_DATE_EPOCH} flag if $SOURCE_DATE_EPOCH is set, or otherwise expanding to nothing (as an arg or no arg to date ...).

we can just inline this and use parameter substitution to accomplish that explicitly and actually quote it

local -a ldflags=($(kube::version::ldflag "buildDate" "$(date ${buildDate} -u +'%Y-%m-%dT%H:%M:%SZ')"))
local -a ldflags
function add_ldflag() {
local key=${1}
local val=${2}
# If you update these, also update the list pkg/version/def.bzl.
ldflags+=(
"-X '${KUBE_GO_PACKAGE}/pkg/version.${key}=${val}'"
"-X '${KUBE_GO_PACKAGE}/vendor/k8s.io/client-go/pkg/version.${key}=${val}'"
"-X '${KUBE_GO_PACKAGE}/pkg/kubectl/version.${key}=${val}'"
)
}

add_ldflag "buildDate" "$(date ${SOURCE_DATE_EPOCH:+"--date=@${SOURCE_DATE_EPOCH}"} -u +'%Y-%m-%dT%H:%M:%SZ')"
if [[ -n ${KUBE_GIT_COMMIT-} ]]; then
ldflags+=($(kube::version::ldflag "gitCommit" "${KUBE_GIT_COMMIT}"))
ldflags+=($(kube::version::ldflag "gitTreeState" "${KUBE_GIT_TREE_STATE}"))
add_ldflag "gitCommit" "${KUBE_GIT_COMMIT}"
add_ldflag "gitTreeState" "${KUBE_GIT_TREE_STATE}"
fi

if [[ -n ${KUBE_GIT_VERSION-} ]]; then
ldflags+=($(kube::version::ldflag "gitVersion" "${KUBE_GIT_VERSION}"))
add_ldflag "gitVersion" "${KUBE_GIT_VERSION}"
fi

if [[ -n ${KUBE_GIT_MAJOR-} && -n ${KUBE_GIT_MINOR-} ]]; then
ldflags+=(
$(kube::version::ldflag "gitMajor" "${KUBE_GIT_MAJOR}")
$(kube::version::ldflag "gitMinor" "${KUBE_GIT_MINOR}")
)
add_ldflag "gitMajor" "${KUBE_GIT_MAJOR}"
add_ldflag "gitMinor" "${KUBE_GIT_MINOR}"
fi

# The -ldflags parameter takes a single string, so join the output.
Expand Down