From f00881b56aadac73ecb05b6bd8d7def1fc667652 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Tue, 21 Feb 2023 15:56:25 -0500 Subject: [PATCH] build: add workflow template for shellcheck + ADR --- docs/decisions/0001-shellcheck.rst | 79 ++++++++++++++++++++++++++++++ workflow-templates/shellcheck.yml | 65 ++++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 docs/decisions/0001-shellcheck.rst create mode 100644 workflow-templates/shellcheck.yml diff --git a/docs/decisions/0001-shellcheck.rst b/docs/decisions/0001-shellcheck.rst new file mode 100644 index 00000000..a2845bcc --- /dev/null +++ b/docs/decisions/0001-shellcheck.rst @@ -0,0 +1,79 @@ + +Adopt ShellCheck for shell script linting +######################################### + +Background +********** + +**Accepted** + +Follow-up issues/PRs: + +* `Add ShellCheck to edx-platform `_ + +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 `_, 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 `_ and `static asset tooling `_, we would like to identify a verification tool and encourage Open edX developers to use it. + +ShellCheck +========== + +`ShellCheck `_ 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 `_. +* It can be installed in one line from various `common package managers `_ including apt and homebrew. +* It can be integrated with `common editors `_ including Vim and VSCode. +* As of the writing of this ADR, it `seems to be actively maintained `_. + +Finally, as a personal anecdote: ShellCheck has been extremely helpful to this ADR's author in `rewriting the edx-platform asset build in Bash `_. 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 `_. + +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 `_. + +* 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 `_ 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?* diff --git a/workflow-templates/shellcheck.yml b/workflow-templates/shellcheck.yml new file mode 100644 index 00000000..63b73560 --- /dev/null +++ b/workflow-templates/shellcheck.yml @@ -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' +