Clean up Bash scripts

[frankgrimes97]

Is your feature request related to a problem? Please describe.

In reviewing the documentation and setup/install scripts I noticed numerous issues in the Bash scripts that could lead to problems/bugs.

ShellCheck is a useful linting tool which can help catch and report them.

e.g. for $ shellcheck scripts/pulsar/prepare_helm_release.sh it reports the following issues:

In scripts/pulsar/prepare_helm_release.sh line 21:
CHART_HOME=$(unset CDPATH && cd $(dirname "${BASH_SOURCE[0]}")/../.. && pwd)
                                ^----------------------------^ SC2046 (warning): Quote this to prevent word splitting.


In scripts/pulsar/prepare_helm_release.sh line 22:
cd ${CHART_HOME}
^--------------^ SC2164 (warning): Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
   ^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
cd "${CHART_HOME}" || exit


In scripts/pulsar/prepare_helm_release.sh line 100:
        kubectl create namespace ${namespace} ${local:+ -o yaml --dry-run=client}
                                 ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        kubectl create namespace "${namespace}" ${local:+ -o yaml --dry-run=client}


In scripts/pulsar/prepare_helm_release.sh line 107:
if [ $? -ne 0 ]; then
     ^-- SC2181 (style): Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?.


In scripts/pulsar/prepare_helm_release.sh line 124:
${CHART_HOME}/scripts/pulsar/generate_token_secret_key.sh -n ${namespace} -k ${release} ${extra_opts}
^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                             ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                                             ^--------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                                                        ^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
"${CHART_HOME}"/scripts/pulsar/generate_token_secret_key.sh -n "${namespace}" -k "${release}" "${extra_opts}"


In scripts/pulsar/prepare_helm_release.sh line 133:
    ${CHART_HOME}/scripts/pulsar/generate_token.sh -n ${namespace} -k ${release} -r ${user} ${extra_opts} 
    ^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                      ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                                      ^--------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                                                    ^-----^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                                                            ^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
    "${CHART_HOME}"/scripts/pulsar/generate_token.sh -n "${namespace}" -k "${release}" -r "${user}" "${extra_opts}" 

For more information:
  https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
  https://www.shellcheck.net/wiki/SC2164 -- Use 'cd ... || exit' or 'cd ... |...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

Describe the solution you'd like

There are GitHub Actions available which run ShellCheck on every Pull Request to help keep the Bash scripts clean and bug-free moving forward.
e.g.

• https://github.com/reviewdog/action-shellcheck
• https://github.com/ludeeus/action-shellcheck

[frankgrimes97]

To clarify further, I suppose the ask is as follows...

Would Pull Request contributions be welcome to:

1. Apply the cleanup to the Bash scripts as per ShellSheck recommendations
2. Add a GitHub Action to run ShellCheck against Bash scripts on new Pull Requests

[lhotari]

To clarify further, I suppose the ask is as follows...

Would Pull Request contributions be welcome to:

1. Apply the cleanup to the Bash scripts as per ShellSheck recommendations

Please go ahead.

2. Add a GitHub Action to run ShellCheck against Bash scripts on new Pull Requests

That could be an overkill for Pulsar Helm chart since scripts are rarely modified. Because of security policy, we need to review third party actions and get them approved by org admins, so that's another reason why it's not desirable when there's not much benefit.

[frankgrimes97]

approved by org admins

FWIW, I did find reference to https://github.com/ludeeus/action-shellcheck in https://github.com/apache/infrastructure-actions (it was updated to the latest release not too long ago on
apache/infrastructure-actions#199)
Might that mean that it's already approved for use?

[frankgrimes97]

I also found the following direct usage (without a custom GitHub Action) of ShellCheck here: https://github.com/apache/maven-wrapper/blob/master/.github/workflows/shellcheck-posix.yml

Perhaps that could be another option.

[frankgrimes97]

@lhotari FYI, I've created a PR (663) with an initial cleanup of the scripts which seems to be passing all CI checks.

As for keeping them clean moving forward, would a custom GitHub Action that both installs and runs ShellCheck as found on the Apache maven-wrapper project be an acceptable compromise?
That's assuming the ASF has not already whitelisted any third-party actions which facilitate running such checks.

Thanks!

[lhotari]

@lhotari FYI, I've created a PR (663) with an initial cleanup of the scripts which seems to be passing all CI checks.

As for keeping them clean moving forward, would a custom GitHub Action that both installs and runs ShellCheck as found on the Apache maven-wrapper project be an acceptable compromise? That's assuming the ASF has not already whitelisted any third-party actions which facilitate running such checks.

Thanks!

The scripts haven't changed over the years many times, that's why I think it's not useful to add the check to CI. Most of the scripts are related to CI itself.