Update shellcheck script to search all files with *sh shebangs #754
Conversation
k8s-ci-robot
added
sig/release
k8s-ci-robot
added
cncf-cla: yes
|
Specific review comment for reference: #740 (comment) |
hack/verify-shellcheck.sh
Outdated
| \( -path ./third_party\* -a -not -path ./third_party/forked\* \) \ | ||
| \)) | ||
| done < <(grep -irl '#!.*sh' . \ | ||
| --exclude-dir={/_\*,/.git\*,/vendor\*,/third_party\*,/third_party/forked\*}) |
There was a problem hiding this comment.
on my macbook I needed to change this to:
| --exclude-dir={/_\*,/.git\*,/vendor\*,/third_party\*,/third_party/forked\*}) | |
| --exclude-dir={/_\*,./.git\*,./vendor\*,./third_party\*,./third_party/forked\*}) |
justaugustus
force-pushed
the
shellcheck
branch
2 times, most recently
from
68fbcec
to
a49f109
Compare
hack/verify-shellcheck.sh
Outdated
| \( -path ./third_party\* -a -not -path ./third_party/forked\* \) \ | ||
| \)) | ||
| done < <(grep -irl '#!.*sh' . \ | ||
| --exclude-dir={/_\*,./.git\*,./vendor\*}) |
There was a problem hiding this comment.
confirmed this works with both BSD (macOS) and gnu grep
otherwise: (without the .)
grep: ./vendor/k8s.io/test-infra/prow/tide/README.md: No such file or directory
grep: ./vendor/k8s.io/test-infra/prow/crier/README.md: No such file or directory
grep: ./vendor/k8s.io/test-infra/prow/plank/README.md: No such file or directory
grep: ./vendor/k8s.io/test-infra/gubernator/github/secrets.py: No such file or directory
There was a problem hiding this comment.
ultimate nit / suggestion: this does fit on a single line, the \ and second line are a little superfluous now :^)
There was a problem hiding this comment.
oh also should there be another . in front of/_\*,?
/hold
There was a problem hiding this comment.
Oops, not quite:
$ grep -irl '#!.*sh' . --exclude-dir={./_\*,./.git\*,./vendor\*}
./changelog-update
./push-build.sh
./hack/verify-shellcheck.sh
./.git/hooks/pre-receive.sample
./.git/hooks/pre-rebase.sample
./.git/hooks/pre-push.sample
./.git/hooks/update.sample
./.git/hooks/pre-commit.sample
./.git/hooks/commit-msg.sample
./.git/hooks/pre-applypatch.sample
./.git/hooks/prepare-commit-msg.sample
./.git/hooks/applypatch-msg.sample
./.git/hooks/post-update.sample
./rpm/entry.sh
./rpm/docker-build.sh
./script-template
./relnotes
./build/_test2/2
./gcbmgr
./_test/1
./debian/jenkins.sh
./debian/bionic/kubeadm/debian/postinst
./debian/bionic/kubectl/debian/postinst
./debian/bionic/kubelet/debian/postinst
./compile-release-tools
./branchff
./release-notify
./anago
./find_green_build
./lib/releaselib_test.sh
./lib/releaselib.sh
./lib/common.sh
./lib/gitlib_test.sh
./lib/gitlib.sh
./prin
I'm poking at it.
There was a problem hiding this comment.
You can see here, that it's checking the .git dir as well: https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/670/pull-kind-verify/1144523142685790208
There was a problem hiding this comment.
yeah, I think this version is suitable for kind: grep -irl '#!.*sh' . --exclude-dir={bin\*,.git\*,vendor\*}, for here grep -irl '#!.*sh' . --exclude-dir={_\*,.git\*,vendor\*} maybe?
There was a problem hiding this comment.
Looks like we've got a winner: grep -irl '#!.*sh' . --exclude-dir={_\*,.git\*,vendor\*}
k8s-ci-robot
added
the
lgtm
|
@BenTheElder -- no, no, thank you! A @kubernetes/bash-firefighters superhero :) |
k8s-ci-robot
added
the
do-not-merge/hold
Several of the shell scripts in this repo do not have `.sh` extensions, which means we will miss shellcheck-ing them with a `find` that considers only the file name. Signed-off-by: Stephen Augustus <[email protected]> Co-Authored-By: Benjamin Elder <[email protected]>
k8s-ci-robot
removed
the
lgtm
There was a problem hiding this comment.
/lgtm
and here's a green run of ~ the same thing for kind https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/670/pull-kind-verify/1144528174026067968
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, justaugustus 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 |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
justaugustus
changed the title
Okay, one more round with this shellcheck script 😄
Several of the shell scripts in this repo do not have
.shextensions, which means we will miss shellcheck-ing them with a
findthat considers only the file name.Signed-off-by: Stephen Augustus [email protected]
/cc @pswica
/assign @spiffxp @BenTheElder
/sig release
/area release-eng
ref: #726, #740, #742, #753