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

Fix shellcheck lint errors in third_party/* scripts #74180

Merged

Conversation

@mrbobbytables
Copy link
Member

mrbobbytables commented Feb 17, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Ensures all scripts under third_party/* pass hack/verify-shellcheck.sh

Which issue(s) this PR fixes:
Working towards #72956

Special notes for your reviewer:
None :)

Does this PR introduce a user-facing change?:

NONE

/sig testing

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Feb 21, 2019

/uncc cblecker jbeda
/assign @BenTheElder

@mrbobbytables

This comment has been minimized.

Copy link
Member Author

mrbobbytables commented Feb 24, 2019

/priority important-longterm

@@ -101,37 +101,37 @@ function juLog() {
# eval the command sending output to a file
outf=/var/tmp/ju$$.txt
errf=/var/tmp/ju$$-err.txt
>${outf}
:>${outf}

This comment has been minimized.

Copy link
@mrbobbytables

mrbobbytables Feb 24, 2019

Author Member

SC2188 - "This redirection doesn't have a command. Move to its command (or use 'true' as no-op)."

Essentially using true as a noop here. As far as I can gather, it's just used to create "${outf}"

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Apr 11, 2019

Member

should we use touch then?

This comment has been minimized.

Copy link
@cblecker

cblecker Apr 11, 2019

Member

I don't think it matters. It's still valid posix

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Apr 12, 2019

Member

It's valid but I think less people will understand this as opposed to touch.

if [ ${err} = 0 -a -n "${ereg:-}" ]; then
H=`echo "${out}" | egrep ${icase} "${ereg}"`
out="$(${SED} -e 's/^\([^+]\)/| \1/g' "$outf")"
if [ ${err} = 0 ] && [ -n "${ereg:-}" ]; then

This comment has been minimized.

Copy link
@mrbobbytables

mrbobbytables Feb 24, 2019

Author Member

For reference - SC2166 - "Prefer [ p ] && [ q ] as [ p -a q ] is not well defined."

@@ -19,13 +19,13 @@ entries="aarch64 aarch64_be alpha arm armeb hppa m68k microblaze microblazeel mi
if [ "${1}" = "--reset" ]; then
shift
(
cd /proc/sys/fs/binfmt_misc
cd /proc/sys/fs/binfmt_misc || { echo "Could not cd to /proc/sys/fs/binfmt_misc"; exit 1; }

This comment has been minimized.

Copy link
@mrbobbytables

mrbobbytables Feb 24, 2019

Author Member

For reference - SC2164 - "Use cd ... || exit in case cd fails."

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Feb 24, 2019

Member

Do we need to move to third_party/forked if we modify this? @spiffxp

@mrbobbytables mrbobbytables force-pushed the mrbobbytables:fix-third-party-shellcheck branch from 0bf556f to 1819bd7 Feb 26, 2019

@mrbobbytables mrbobbytables force-pushed the mrbobbytables:fix-third-party-shellcheck branch from 1819bd7 to aeaf902 Mar 1, 2019

@mrbobbytables

This comment has been minimized.

Copy link
Member Author

mrbobbytables commented Mar 1, 2019

/retest

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Mar 11, 2019

/assign @spiffxp
these changes look good, but I'm not sure if we should change some of these without moving them to forked? or how that is handled more generally

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Mar 23, 2019

these changes look good, but I'm not sure if we should change some of these without moving them to forked? or how that is handled more generally

/assign @thockin
I need to page in @thockin to answer this, and he's in third_party/OWNERS anyway... does modifying something in third_party/ mean we need to move it to third_party/forked/?

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 23, 2019

Long story short, yes. These changes seem correct, but are they really worth doing? Eventually we will re-import from upstream and this will just be friction.

If we have real bugs, A+, let's do it. Otherwise it seems like very low ROI. Convince me?

@mrbobbytables

This comment has been minimized.

Copy link
Member Author

mrbobbytables commented Mar 23, 2019

This is a part of working towards enabling shellcheck linting for all shell scripts.

We've modified the the ones we've forked a fair amount, and I would suggest at least fixing/enabling a linting check for those.

However, I do think the case can be made to ignore the ones we haven't forked or made any significant changes to such as third_party/intemp and /third_party/multiarch/qemu-user-static.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Mar 23, 2019

@mrbobbytables

This comment has been minimized.

Copy link
Member Author

mrbobbytables commented Mar 24, 2019

@thockin I saw your reply before seeing @spiffxp's comment specifically asking about moving things from third_party to third_party/forked. 🤦‍♂️ My bad, sorry about that. Thought it was more in general.

My thought is pretty much the same though -- thinking fix/lint/comment things in third_party/forked (really just shell2junit).

Then update hack/verify-shellcheck.sh to exclude third_party/ while including third_party/forked/* -- limiting it to anything that we fork & modify.

@mrbobbytables mrbobbytables force-pushed the mrbobbytables:fix-third-party-shellcheck branch from aeaf902 to 9e719d8 Apr 11, 2019

@mrbobbytables

This comment has been minimized.

Copy link
Member Author

mrbobbytables commented Apr 11, 2019

/retest

@cblecker
Copy link
Member

cblecker left a comment

/lgtm
/approve

@@ -101,37 +101,37 @@ function juLog() {
# eval the command sending output to a file
outf=/var/tmp/ju$$.txt
errf=/var/tmp/ju$$-err.txt
>${outf}
:>${outf}

This comment has been minimized.

Copy link
@cblecker

cblecker Apr 11, 2019

Member

I don't think it matters. It's still valid posix

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, mrbobbytables

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Apr 12, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Apr 12, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 5b434ee into kubernetes:master Apr 12, 2019

18 checks passed

cla/linuxfoundation mrbobbytables authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Context retired. Status moved to "pull-kubernetes-dependencies".
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.