Use shellcheck (via pantsbuild) #5751

cognifloyd · 2022-10-05T03:50:51Z

Background

This is another part of introducing pants, as discussed in the TSC Meetings on 12 July 2022, 02 Aug 2022, 06 Sept 2022, and 04 Oct 2022. Pants has fine-grained per-file caching of results for lint, fmt (like black), test, etc. It also has lockfiles that work well for monorepos that have multiple python packages. With these lockfiles CI should not break when any of our dependencies or our transitive dependencies release new versions, because CI will continue to use the locked version until we explicitly relock with updates.

To keep PRs as manageable/reviewable as possible, introducing pants will take a series of PRs. I do not know yet how many PRs; I will break this up into logical steps with these goals:

introduce pants to the st2 repo, and
teach some (but not all) TSC members about pants step-by-step.

Other pants PRs include:

Overview of this PR

It is helpful to review each commit in this PR individually.

6b2f993 Rename shell targets to be a bit more consistent.
d442fd5 Add/adjust targets for executable files/scripts
- In Create pantsbuild BUILD files with ./pants tailor :: #5738, tailor added a lot of metadata, but it could not identify some of our extension-less scripts as shell or python.
- So, we add metadata for all of those scripts (basically anything in the bin/ directories).
- This way, pants can run shellcheck on all of our shell files, not just *.sh.
854873c Add shellcheck (skipping anything that does not pass)
- Enable the shellcheck backend in pants (pants.toml: [GLOBAL].backend_packages).
  - We enabled language backends in Enable pantsbuild language backends #5725 which included the shell_sources and shell_source targets.
  - Now, with the shellcheck backend, ./pants lint :: will run shellcheck on all files in those shell_source/s targets.
- The Makefile only runs shellcheck on scripts/ci/*.sh, scripts/github/*.sh, and scripts/*.sh.
- So, tell pants to skip running shellcheck any of the shell files that don't currently pass.
- This still increases our shellcheck coverage; Someone can fix other files in follow-up PRs (maybe hacktoberfest or other first-time contributors could do it).
- Also demonstrates overrides: a fairly new--and awesome--feature in pants.
- It allows you to add very specific metadata about individual files without adding extra, possibly-overlapping, targets to the BUILD files. It also avoids adding that metadata to more files than is required.

Relevant Pants documentation

./pants lint goal
Shell Overview
shellcheck backend
individual source targets:
- python_source
- shell_source
multiple sources targets (aka target generators):
- python_sources
  - overrides field
- shell_sources
  - overrides field
An example that uses the overrides field in the linters/formatters docs
Video about how pants facilitates "Incrementally Adopting New Tools"

Things you can do with pantsbuild

This is the first PR that adds a lint backend. So, now you can run ./pants lint :: (the same command that the new GHA Lint workflow runs).

You could do something like ./pants lint scripts/*.sh if you only want to lint some of our scripts.

That looks like this:

./pants lint scripts/*.sh   
22:38:01.83 [INFO] Completed: Lint with Shellcheck - shellcheck succeeded.

✓ shellcheck succeeded.

$ ./pants lint ::
22:39:34.11 [INFO] Completed: Lint with Shellcheck - shellcheck succeeded.

✓ shellcheck succeeded.

To see the shellcheck errors for a skipped file--I'll use st2common/bin/st2ctl for example--I commented out the skip_shellcheck=True, line in the BUILD file. That shows what output looks like for failures:

$ ./pants lint st2common/bin/st2ctl            
22:45:53.16 [ERROR] Completed: Lint with Shellcheck - shellcheck failed (exit code 1).

In st2common/bin/st2ctl line 23:
[ -r /etc/environment ] && source /etc/environment
                                  ^--------------^ SC1091 (info): Not following: /etc/environment was not specified as input (see shellcheck -x).


In st2common/bin/st2ctl line 27:
[ -r /etc/default/st2ctl ] && source /etc/default/st2ctl
                                     ^-----------------^ SC1091 (info): Not following: /etc/default/st2ctl was not specified as input (see shellcheck -x).


In st2common/bin/st2ctl line 29:
[ -r /etc/sysconfig/st2ctl ] && source /etc/sysconfig/st2ctl
                                       ^-------------------^ SC1091 (info): Not following: /etc/sysconfig/st2ctl was not specified as input (see shellcheck -x).


In st2common/bin/st2ctl line 66:
    if [ $(id -u) -ne 0 ]; then
         ^------^ SC2046 (warning): Quote this to prevent word splitting.


In st2common/bin/st2ctl line 75:
  if [ -z ${COM} ]; then
          ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
  if [ -z "${COM}" ]; then


In st2common/bin/st2ctl line 97:
    service_manager ${COM} start
                    ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    service_manager "${COM}" start


In st2common/bin/st2ctl line 104:
    service_manager ${COM} stop
                    ^----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    service_manager "${COM}" stop


In st2common/bin/st2ctl line 112:
    if [ -z $SYSTEMD_RELOADED ]; then
            ^---------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    if [ -z "$SYSTEMD_RELOADED" ]; then


In st2common/bin/st2ctl line 117:
    systemctl $action $svcname
              ^-----^ SC2086 (info): Double quote to prevent globbing and word splitting.
                      ^------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    systemctl "$action" "$svcname"


In st2common/bin/st2ctl line 118:
  elif [ $(cat /proc/1/comm) = init ] && (/sbin/initctl version 2>/dev/null | grep -q upstart) &&
         ^-----------------^ SC2046 (warning): Quote this to prevent word splitting.


In st2common/bin/st2ctl line 119:
          [ -f /etc/init/${svcname}.conf ]; then
                         ^--------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
          [ -f /etc/init/"${svcname}".conf ]; then


In st2common/bin/st2ctl line 123:
    /sbin/initctl $action $svcname
                  ^-----^ SC2086 (info): Double quote to prevent globbing and word splitting.
                          ^------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    /sbin/initctl "$action" "$svcname"


In st2common/bin/st2ctl line 125:
    service $svcname $action
            ^------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                     ^-----^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    service "$svcname" "$action"


In st2common/bin/st2ctl line 137:
  PID=`ps ax | grep -v grep | grep -v st2ctl | grep -E "(${COM}\.wsgi)|(bin/${COM})|(hubot .*${COM})" | awk '{print $1}'`
      ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
       ^-- SC2009 (info): Consider using pgrep instead of grepping ps output.

Did you mean: 
  PID=$(ps ax | grep -v grep | grep -v st2ctl | grep -E "(${COM}\.wsgi)|(bin/${COM})|(hubot .*${COM})" | awk '{print $1}')


In st2common/bin/st2ctl line 138:
  if [[ ! -z ${PID} ]]; then
        ^-- SC2236 (style): Use -n instead of ! -z.


In st2common/bin/st2ctl line 141:
      kill -USR1 ${p}
                 ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
      kill -USR1 "${p}"


In st2common/bin/st2ctl line 154:
  flags="${@}"
        ^----^ SC2124 (warning): Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.


In st2common/bin/st2ctl line 156:
  if [ ! -z ${1} ]; then
       ^-- SC2236 (style): Use -n instead of ! -z.
            ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
  if [ ! -z "${1}" ]; then


In st2common/bin/st2ctl line 170:
  if [ -z ${1} ]; then
          ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
  if [ -z "${1}" ]; then


In st2common/bin/st2ctl line 172:
  elif [ ${1} == '--verbose' ] && [ -z ${2} ]; then
         ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                       ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
  elif [ "${1}" == '--verbose' ] && [ -z "${2}" ]; then


In st2common/bin/st2ctl line 179:
  st2-register-content --config-file ${ST2_CONF} ${REGISTER_FLAGS}
                                                 ^---------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
  st2-register-content --config-file ${ST2_CONF} "${REGISTER_FLAGS}"


In st2common/bin/st2ctl line 194:
  COMPONENTS=${COMPONENTS}
  ^--------^ SC2269 (info): This variable is assigned to itself, so the assignment does nothing.


In st2common/bin/st2ctl line 197:
    PID=`ps ax | grep -v grep | grep -v st2ctl | grep -E "(${COM}\.wsgi)|(bin/${COM})|(hubot .*${COM})" | awk '{print $1}'`
        ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
         ^-- SC2009 (info): Consider using pgrep instead of grepping ps output.

Did you mean: 
    PID=$(ps ax | grep -v grep | grep -v st2ctl | grep -E "(${COM}\.wsgi)|(bin/${COM})|(hubot .*${COM})" | awk '{print $1}')


In st2common/bin/st2ctl line 199:
    if [[ ! -z ${PID} ]]; then
          ^-- SC2236 (style): Use -n instead of ! -z.


In st2common/bin/st2ctl line 229:
    validate_in_components ${2}
                           ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    validate_in_components "${2}"


In st2common/bin/st2ctl line 230:
    service_manager ${2} restart
                    ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    service_manager "${2}" restart


In st2common/bin/st2ctl line 235:
    validate_in_components ${2}
                           ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    validate_in_components "${2}"


In st2common/bin/st2ctl line 236:
    if reopen_component_log_files ${2}; then
                                  ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean: 
    if reopen_component_log_files "${2}"; then


In st2common/bin/st2ctl line 242:
    register_content ${@:2}
                     ^----^ SC2068 (error): Double quote array expansions to avoid re-splitting elements.


In st2common/bin/st2ctl line 253:
    read verify
    ^--^ SC2162 (info): read without -r will mangle backslashes.


In st2common/bin/st2ctl line 258:
      register_content ${@:2}
                       ^----^ SC2068 (error): Double quote array expansions to avoid re-splitting elements.

For more information:
  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
  https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
  https://www.shellcheck.net/wiki/SC2124 -- Assigning an array to a string! A...



✕ shellcheck failed.

cognifloyd · 2022-10-05T04:59:12Z

contrib/examples/actions/bash_random/BUILD

+shell_sources(
+    overrides={
+        "random2.sh": {"skip_shellcheck": True},


This was a very simple way I could show off the overrides feature of pants.
random.sh passes shellcheck, but random2.sh does not. This way we skip running shellcheck on as few files as possible.

cognifloyd · 2022-10-05T05:01:23Z

st2common/bin/BUILD

+python_sources(
+    sources=["*.py", "st2*", "!st2ctl", "!st2-self-check", "!st2-run-pack-tests"],
+)
+
+shell_sources(
+    name="shell",
+    sources=["st2ctl", "st2-self-check", "st2-run-pack-tests"],


Notice how I've got the st2* glob on python_sources.
That would match the files from shell_sources, so we exclude them from matching using !.

Most of the shell_source(s) targets were created with `./pants tailor ::`. That missed a few of our scripts that do not have a `.sh` extension, as well as a few executable python scripts that did not have a `.py` extension, so we let pants know about them.

This excludes some scripts from shellcheck because they weren't linted previously. We can clean them up in one or more follow-up PRs and remove `skip_shellcheck=True` from the relevant BUILD targets to re-enable shellcheck for those files.

cognifloyd · 2022-10-08T19:17:19Z

You can see the shellcheck run in CI here: https://github.com/StackStorm/st2/actions/runs/3211491875/jobs/5249698302

rush-skills

LGTM

amanda11

Minor comment, just to check todo is still relevant. But if so, then ok from me to merge.

amanda11 · 2022-10-10T12:15:55Z

st2common/bin/migrations/v3.5/BUILD

@@ -1 +1,3 @@
+# TODO: what to do about st2-migrate-db-dict-field-values ?


Is this to be resolved in another PR? Just checking this TODO is still relevant.

Yeah. The TODO is still relevant. I'm not sure how best to handle the symlink - so I'm punting. 🤷

cognifloyd added infrastructure: ci/cd maintenance pantsbuild labels Oct 5, 2022

cognifloyd added this to the pants milestone Oct 5, 2022

cognifloyd requested review from winem, arm4b, nzlosh, rush-skills, amanda11 and a team October 5, 2022 03:50

cognifloyd self-assigned this Oct 5, 2022

pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Oct 5, 2022

cognifloyd force-pushed the pants-shellcheck branch 2 times, most recently from 1729d45 to 36de7b0 Compare October 5, 2022 04:08

cognifloyd commented Oct 5, 2022

View reviewed changes

cognifloyd marked this pull request as draft October 5, 2022 17:48

cognifloyd mentioned this pull request Oct 5, 2022

Add GHA workflow to lint with pantsbuild #5758

Merged

cognifloyd force-pushed the pants-shellcheck branch from 36de7b0 to 81d053c Compare October 5, 2022 18:04

pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/L PR that changes 100-499 lines. Requires some effort to review. labels Oct 5, 2022

cognifloyd marked this pull request as ready for review October 5, 2022 18:08

This comment was marked as resolved.

Sign in to view

Eric-Arellano approved these changes Oct 5, 2022

View reviewed changes

cognifloyd added 4 commits October 8, 2022 14:12

pants: Rename shell targets

b1b3b3c

update changelog entry

beae45f

cognifloyd force-pushed the pants-shellcheck branch from 81d053c to beae45f Compare October 8, 2022 19:13

cognifloyd enabled auto-merge October 8, 2022 19:15

rush-skills approved these changes Oct 9, 2022

View reviewed changes

amanda11 approved these changes Oct 10, 2022

View reviewed changes

cognifloyd merged commit 11989ed into master Oct 10, 2022

cognifloyd deleted the pants-shellcheck branch October 10, 2022 12:19

This was referenced Oct 13, 2022

Use black (via pantsbuild) #5774

Merged

Use flake8 (via pantsbuild) #5776

Merged

Use bandit (via pantsbuild) #5777

Merged

Use regex-lint to provide circular dependency checking in pants #5778

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use shellcheck (via pantsbuild) #5751

Use shellcheck (via pantsbuild) #5751

cognifloyd commented Oct 5, 2022 •

edited

Loading

cognifloyd Oct 5, 2022 •

edited

Loading

cognifloyd Oct 5, 2022

This comment was marked as resolved.

cognifloyd commented Oct 8, 2022

rush-skills left a comment

amanda11 left a comment

amanda11 Oct 10, 2022

cognifloyd Oct 10, 2022

		@@ -1 +1,3 @@
		# TODO: what to do about st2-migrate-db-dict-field-values ?

Use shellcheck (via pantsbuild) #5751

Use shellcheck (via pantsbuild) #5751

Conversation

cognifloyd commented Oct 5, 2022 • edited Loading

Background

Overview of this PR

Relevant Pants documentation

Things you can do with pantsbuild

cognifloyd Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

cognifloyd Oct 5, 2022

Choose a reason for hiding this comment

This comment was marked as resolved.

cognifloyd commented Oct 8, 2022

rush-skills left a comment

Choose a reason for hiding this comment

amanda11 left a comment

Choose a reason for hiding this comment

amanda11 Oct 10, 2022

Choose a reason for hiding this comment

cognifloyd Oct 10, 2022

Choose a reason for hiding this comment

cognifloyd commented Oct 5, 2022 •

edited

Loading

cognifloyd Oct 5, 2022 •

edited

Loading