Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add ADR to adopt ShellCheck #60

Merged
merged 9 commits into from
Mar 2, 2023
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>`_
kdmccormick marked this conversation as resolved.
Show resolved Hide resolved

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.
kdmccormick marked this conversation as resolved.
Show resolved Hide resolved

* 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 may stick with the default provided in this repo or can override to a different version. Those that override it 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.
kdmccormick marked this conversation as resolved.
Show resolved Hide resolved

* 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 tentative 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 documentation, 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 on 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?*