Prow: Add shellcheck.sh task

[alejovicu]

Add shell script to run the shell check task.

This task will be executed by Prow:
openshift/release#1131

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alejovicu
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yifan-gu

If they are not already assigned, you can assign the PR to them by writing /assign @yifan-gu in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci-robot]

Hi @alejovicu. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[wking]

Maybe update .travis.yaml to use this script? Travis has shellcheck installed by default since 2016 (travis-ci/travis-cookbooks#801, koalaman/shellcheck#1121), so we should be able to skip the container there.

[wking]

Actually, your script has both the container form and the non-container form, so definitely update .travis.yaml ;).

[wking]

Do we need the newline guard? I don't expect us to have any files ending in .sh with shell-sensitive characters in the names. I expect we can also drop the tmp-search business and just paste in the:

for file in $(find /workdir/ -type f -name "*.sh"); do

from .travis.yaml.

[wking]

In fact, we can probably just do:

shellcheck --format=gcc $(find "${DIR}" -type f -name '*.sh')

and skip the for loop entirely (although this diverges from .travis.yaml, so I'm fine punting it to follow-up work).

If we replace /workdir/ with "${DIR}" in the find (to allow folks to call this locally without spinning up a container), you'd want to adjust the container call to:

docker run -e IS_CONTAINER='TRUE' --rm -v "$(realpath "${DIR}")":/workdir:ro --entrypoint sh quay.io/coreos/shellcheck-alpine:v0.5.0 /workdir/hack/shellcheck.sh /workdir;

and add a leading:

DIR="${1:-.}"

or similar to the script (realpath is not in POSIX, but it is in coreutils).

[wking]

This file needs the executable bit set to avoid:

$ ./hack/shellcheck.sh
/home/travis/.travis/job_stages: line 78: ./hack/shellcheck.sh: Permission denied

[wking]

Can you rebase this into a single commit on top of master? Also probably drop your yamllint whitespace fixes in .travis.yaml to avoid conflicting with #101.

[wking]

I don't think we need to print a success message. Exiting zero and the lack of warnings should be sufficient feedback. We also want to exit nonzero of the find exits nonzero. How about:

find "${DIR}" -type f -name '*.sh' -exec shellcheck --format=gcc {} \+ || exit 1

without the if? Even the || exit 1 may be optional, because with:

if [ "$IS_CONTAINER" != "" ]; then
  find "${DIR}" -type f -name '*.sh' -exec shellcheck --format=gcc {} \+
else
  docker run ...
fi

the find command will be the last command in the script, so the scripts default exit handling will pass it back to the caller.

[alejovicu]

This success message is to get an execution feedback from the pod log in Prow when it passed.

[wking]

This success message is to get an execution feedback from the pod log in Prow when it passed.

If Prow needs something on stdout for success, you could always use:

./hack/shellcheck.sh && echo 'Shell Check Passed'

or similar in openshift/release#1131.