Skip to content

Commit

Permalink
build: add workflow template for shellcheck + ADR
Browse files Browse the repository at this point in the history
  • Loading branch information
kdmccormick committed Feb 22, 2023
1 parent 78e78ab commit f00881b
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 0 deletions.
79 changes: 79 additions & 0 deletions docs/decisions/0001-shellcheck.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@

Adopt ShellCheck for shell script linting
#########################################

Background
**********

**Accepted**

Follow-up issues/PRs:

* `Add ShellCheck to edx-platform <https://github.com/openedx/edx-platform/pull/31809>`_

Context
*******

Background
==========

Although Python is the preferred language for backend development in the Open edX ecosytem, there are a few circumstances in which shell scripting (generally: Bash) is a superior tool. In particular, developers may reach for shell scripting languages when one or more of the following is true:

* A consistent Python environment is not guaranteed.
* Requiring a Python environment would make the surrounding process (a build, a deployment pipeline, etc) less efficient.
* The operation is centered around moving or copying files, transforming text streams, or otherwise using standard POSIX tools.

In spite of these benefits, `shell scripting is notoriously fraught with pitfalls <https://mywiki.wooledge.org/BashPitfalls>`_, and many Open edX developers will have less experience writing shell scripts than they do writing Python or JavaScript. So, it is understood that shell script usage should be limited to specific scenarios.

In almost all repositories, Open edX Python code is checked for common mistakes using pylint. Many repositories go further and use static analysis tools such as pycodestyle, pydocstyle, mypy, black, and isort. In constrast, even in spite of the known difficulty of correct shell scripting, Open edX shell scripts are *not* currently checked with any static analysis tools. Therefore, as shell scripts continue to be used in new Open edX contexts like `translations tooling <https://github.com/openedx/openedx-atlas>`_ and `static asset tooling <https://github.com/openedx/edx-platform/pull/31790>`_, we would like to identify a verification tool and encourage Open edX developers to use it.

ShellCheck
==========

`ShellCheck <https://shellcheck.net>`_ seems very suitable:

* It is well-known, with over 31.5k stars on GitHub. Pylint, in comparison, has 4.5k.
* Its error codes, including suggested resolution steps, are `documented on its wiki <https://www.shellcheck.net/wiki/SC1000>`_.
* It can be installed in one line from various `common package managers <https://github.com/koalaman/shellcheck#user-content-installing>`_ including apt and homebrew.
* It can be integrated with `common editors <https://github.com/koalaman/shellcheck#user-content-in-your-editor>`_ including Vim and VSCode.
* As of the writing of this ADR, it `seems to be actively maintained <https://github.com/koalaman/shellcheck/commits/master>`_.

Finally, as a personal anecdote: ShellCheck has been extremely helpful to this ADR's author in `rewriting the edx-platform asset build in Bash <https://github.com/openedx/edx-platform/pull/31791>`_. In addition to catching simple mistakes like typos, ShellCheck's detailed error pages have provided understanding of important shell concepts such as `globbing and word splitting <https://www.shellcheck.net/wiki/SC2086>`_.

Decision
********

Open edX developers are encouraged to use ShellCheck as part of the CI suite for repositories containing shell scripts.

Consequences
************

* A configurable ShellCheck workflow template will be added to this repository. This will let developers add ShellChecking to their repository's PRs with a couple clicks.

* In order to ensure deterministic builds, the template will pin ShellCheck to a specific version (v0.9.0 at time of writing). The maintainer of this repository will strive to update the template's version pin over time. Maintainers of repositories using ShellCheck will be encouraged to update their pin over time. We expect that not all maintainers will do this, leading to lag & drift between versions; we expect that this will be OK, given that Bash itself is essentially feature-complete, so any newly-introcuded ShellCheck warnings are not expected to be critical.

* The new template and this ADR will be announced on the forums and Slack.

* ShellCheck will continue to be used in `openedx-atlas <https://github.com/openedx/openedx-atlas>`_.

* The new ShellCheck template will be used to add checking to edx-platform PRs and master. Existing violations will be resolved or granted amnesty (that is: disabled with a cautionary comment).

* We will share this ADR with the Maintainership Pilot team, which has been compiling a tenative list of repository standards, so that they may consider ShellCheck as a standard check for applicable repositories.


Rejected Alternatives
*********************

* **Retroactive application:** We do not plan to retroactively apply ShellCheck to every repository with shell scripts. This would require resolving or applying amnesty to all existing violations, which does not seems worthwhile at this time.

* **Alternative linting tools:** We did not find any ShellCheck-like tools with comparable documtation, popularity, or ubiquity.

* **ShellCheck in the Python cookiecutters:** We could add the ShellCheck workflow to edx-cookiecutters so that it is run on PRs for all new repositories. However, most new repositories will not need shell scripts, so we do not plan doing this.


Potential Future Work
*********************

* For shell script *testing*, `ShellSpec <https://github.com/shellspec/shellspec>`_ looks interesting, although it is not as obviously mature as ShellCheck. This ADR considers its adoption out of scope, but future ADRs may consider taking a position on ShellSpec adoption. Individual repositories are, of course, welcome to try and evaluate it.

* A repository health check could be added with logic along the lines of: *Does this repo have shell scripts? If so, does it run ShellCheck?*
65 changes: 65 additions & 0 deletions workflow-templates/shellcheck.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# ShellCheck is a linter for your shell scripts:
# https://www.shellcheck.net/
# This workflow runs it on PRs and pushes to $default-branch

name: ShellCheck

on:
pull_request:
push:
branches:
- $default-branch

permissions:
contents: read

jobs:

shellcheck:
name: Run ShellCheck
runs-on: ubuntu-latest

steps:

- name: Check out branch
uses: actions/checkout@v3

# Run ShellCheck using a predefine action:
# https://github.com/marketplace/actions/shellcheck
- name: Run ShellCheck
uses: ludeeus/action-shellcheck@master
env:

# We pin to a specific version of ShellCheck so that your build doesn't
# break as newer versions with more warnings are released.
# Maintainers: Keep an eye out for newer ShellCheck versions and consider
# upgrading to them when they are released:
# https://github.com/koalaman/shellcheck/tags
version: v0.9.0

# Severity levels, in increasing order of strictness:
# error
# warning
# info
# style
# We recommend `style` for maximum coverage, but adjust as you see fit.
severity: style

# Add any custom shellcheck CLI options here.
# For example, use `-e SC2059` to ignore a certain warning.
# (However, it's usually to ignore individual warnings inline: `# shellcheck: disable=SC2059`)
SHELLCHECK_OPTS:

# Ignore filepaths or filenames.
# Each is a single string, space-separated.
ignore_paths:
ignore_names:

# By default, your whole repo is scanned for shell scripts.
# Uncomment the next line if you want to limit to a certain directory.
#scandir: './scripts'

# This ensures that all .sh files are passed to shellcheck in one go, making
# ShellCheck aware of "include" logic (`source ./constants.sh`) between scripts.
check_together: 'yes'

0 comments on commit f00881b

Please sign in to comment.