Skip to content

Commit

Permalink
Target specific scripts in "Check Scripts" template
Browse files Browse the repository at this point in the history
Background
----------

Previously, the approach taken in the "Check Scripts" template was to recursively search the entire repository for shell
scripts. There were several problems with that system:

The `-regextype` flag used in the `find` commands of the `shell:check` and `shell:check-mode` tasks is not supported by
the BSD/macOS version of `find`, meaning the tasks would fail if a contributor using a macOS machine tried to run them:

```
find: -regextype: unknown primary or operator
```

The `shell:check` and `shell:check-mode` tasks only provided coverage for files with `.sh` and `.bash` file extensions,
whereas the practice of omitting a file extension on shell scripts is unfortunately quite common.

There was no obvious way to exclude paths of externally maintained files from coverage by the `shell:format` task.

Alternative Solutions
---------------------

The first of the problems listed above could be overcome by configuring the tasks to adjust the `find` commands to use
different flags depending on the operating system of the machine.

The second could be overcome by using `shfmt --files` (which searches recursively for shell scripts by checking for the
presence of a shebang inside files and outputs a list of the paths) as a replacement for `find`.

The third could be overcome by documenting the usage of shfmt's poorly advertised feature of ignoring the paths assigned
an `ignore` attribute in the .editorconfig file.

Chosen Solution
---------------

After careful consideration, the decision was made to abandon the previous approach of attempting to automatically
discover script files and instead instead use the strategy of configuring the template with the specific paths of the
scripts to be checked for each project it is installed into. Although such an approach would not be appropriate in the
case of a check on files that tend to be present in greater abundance and regularly added and moved in a project, this
is not the case for shell scripts in Arduino projects. A project is more likely to contain a few scripts at most and
their paths tend to be reasonably stable.

Code Duplication in Workflow
----------------------------

In order to make it easier to interpret results, navigate logs, and reduce duration of workflow runs, a separate
workflow job is used for each of the distinct checks. Unfortunately it is necessary for the project maintainer to
configure the script paths redundantly in the matrix of each of the three jobs.

Intuitively we would expect this could be avoided by defining the paths at workflow scope via the env key. However, this
is not feasible due to the env context not being available for use in the jobs.<job name>.strategy.matrix key.

It is technically possible to accomplish this by adding a job that converts the data from the env context into a job
output (the `needs` context is available for use in the `matrix` key). However the significant increase in complexity
this would bring to the workflow outweighs the benefit of avoiding duplication.
  • Loading branch information
per1234 committed Nov 30, 2023
1 parent e9c50c4 commit 1098496
Show file tree
Hide file tree
Showing 5 changed files with 188 additions and 101 deletions.
34 changes: 30 additions & 4 deletions .github/workflows/check-shell-task.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ jobs:
echo "result=$RESULT" >> $GITHUB_OUTPUT
lint:
name: ${{ matrix.configuration.name }}
name: ${{ matrix.configuration.name }} (${{ matrix.script }})
needs: run-determination
if: needs.run-determination.outputs.result == 'true'
runs-on: ubuntu-latest
Expand All @@ -75,6 +75,8 @@ jobs:
# ShellCheck's "tty" output format is most suitable for humans reading the log.
format: tty
continue-on-error: false
script:
- other/installation-script/install.sh

steps:
- name: Set environment variables
Expand Down Expand Up @@ -118,15 +120,23 @@ jobs:
continue-on-error: ${{ matrix.configuration.continue-on-error }}
with:
linters: gcc
run: task --silent shell:check SHELLCHECK_FORMAT=${{ matrix.configuration.format }}
run: task --silent shell:check SCRIPT_PATH="${{ matrix.script }}" SHELLCHECK_FORMAT=${{ matrix.configuration.format }}

formatting:
name: formatting (${{ matrix.script }})
needs: run-determination
if: needs.run-determination.outputs.result == 'true'
runs-on: ubuntu-latest
permissions:
contents: read

strategy:
fail-fast: false

matrix:
script:
- other/installation-script/install.sh

steps:
- name: Set environment variables
run: |
Expand Down Expand Up @@ -162,18 +172,30 @@ jobs:
echo "${{ env.SHFMT_INSTALL_PATH }}" >> "$GITHUB_PATH"
- name: Format shell scripts
run: task --silent shell:format
run: |
task \
--silent \
shell:format \
SCRIPT_PATH="${{ matrix.script }}"
- name: Check formatting
run: git diff --color --exit-code

executable:
name: executable (${{ matrix.script }})
needs: run-determination
if: needs.run-determination.outputs.result == 'true'
runs-on: ubuntu-latest
permissions:
contents: read

strategy:
fail-fast: false

matrix:
script:
- other/installation-script/install.sh

steps:
- name: Checkout repository
uses: actions/checkout@v4
Expand All @@ -185,4 +207,8 @@ jobs:
version: 3.x

- name: Check for non-executable scripts
run: task --silent shell:check-mode
run: |
task \
--silent \
shell:check-mode \
SCRIPT_PATH="${{ matrix.script }}"
80 changes: 33 additions & 47 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ tasks:
- task: python:lint
- task: python:test
- task: shell:check
vars:
SCRIPT_PATH: other/installation-script/install.sh
- task: shell:check-mode
vars:
SCRIPT_PATH: other/installation-script/install.sh
- task: yaml:lint

fix:
Expand All @@ -49,6 +53,8 @@ tasks:
- task: markdown:fix
- task: python:format
- task: shell:format
vars:
SCRIPT_PATH: other/installation-script/install.sh

ci:sync:
desc: Sync CI workflows from templates
Expand All @@ -64,7 +70,6 @@ tasks:
"{{.WORKFLOW_TEMPLATES_PATH}}/check-npm-task.yml" \
"{{.WORKFLOW_TEMPLATES_PATH}}/check-prettier-formatting-task.yml" \
"{{.WORKFLOW_TEMPLATES_PATH}}/check-python-task.yml" \
"{{.WORKFLOW_TEMPLATES_PATH}}/check-shell-task.yml" \
"{{.WORKFLOW_TEMPLATES_PATH}}/check-taskfiles.yml" \
"{{.WORKFLOW_TEMPLATES_PATH}}/check-yaml-task.yml" \
"{{.WORKFLOW_TEMPLATES_PATH}}/sync-labels-npm.yml" \
Expand Down Expand Up @@ -832,81 +837,62 @@ tasks:
cmds:
- poetry run pytest workflow-templates/assets/deploy-mkdocs-versioned/siteversion/tests

# Parameter variables:
# - SCRIPT_PATH: path of the script to be checked.
# Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml
shell:check:
desc: Check for problems with shell scripts
cmds:
- |
if [[ "{{.SCRIPT_PATH}}" == "" ]]; then
echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable."
echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage"
exit 1
fi
- |
if ! which shellcheck &>/dev/null; then
echo "shellcheck not installed or not in PATH."
echo "Please install: https://github.com/koalaman/shellcheck#installing"
exit 1
fi
- |
# There is something odd about shellcheck that causes the task to always exit on the first fail, despite any
# measures that would prevent this with any other command. So it's necessary to call shellcheck only once with
# the list of script paths as an argument. This could lead to exceeding the maximum command length on Windows if
# the repository contained a large number of scripts, but it's unlikely to happen in reality.
shellcheck \
--format={{default "tty" .SHELLCHECK_FORMAT}} \
$(
# The odd method for escaping . in the regex is required for windows compatibility because mvdan.cc/sh gives
# \ characters special treatment on Windows in an attempt to support them as path separators.
find . \
-type d -name '.git' -prune -or \
-type d -name '.licenses' -prune -or \
-type d -name '__pycache__' -prune -or \
-type d -name 'node_modules' -prune -or \
\( \
-regextype posix-extended \
-regex '.*[.](bash|sh)' -and \
-type f \
\) \
-print
)
"{{.SCRIPT_PATH}}"
# Parameter variables:
# - SCRIPT_PATH: path of the script to be checked.
# Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml
shell:check-mode:
desc: Check for non-executable shell scripts
cmds:
- |
EXIT_STATUS=0
while read -r nonExecutableScriptPath; do
# The while loop always runs once, even if no file was found
if [[ "$nonExecutableScriptPath" == "" ]]; then
continue
fi
echo "::error file=${nonExecutableScriptPath}::non-executable script file: $nonExecutableScriptPath";
EXIT_STATUS=1
done <<<"$(
# The odd approach to escaping `.` in the regex is required for windows compatibility because mvdan.cc/sh
# gives `\` characters special treatment on Windows in an attempt to support them as path separators.
find . \
-type d -name '.git' -prune -or \
-type d -name '.licenses' -prune -or \
-type d -name '__pycache__' -prune -or \
-type d -name 'node_modules' -prune -or \
\( \
-regextype posix-extended \
-regex '.*[.](bash|sh)' -and \
-type f -and \
-not -executable \
-print \
\)
)"
exit $EXIT_STATUS
if [[ "{{.SCRIPT_PATH}}" == "" ]]; then
echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable."
echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage"
exit 1
fi
- |
test -x "{{.SCRIPT_PATH}}"
# Parameter variables:
# - SCRIPT_PATH: path of the script to be formatted.
# Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml
shell:format:
desc: Format shell script files
cmds:
- |
if [[ "{{.SCRIPT_PATH}}" == "" ]]; then
echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable."
echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage"
exit 1
fi
- |
if ! which shfmt &>/dev/null; then
echo "shfmt not installed or not in PATH. Please install: https://github.com/mvdan/sh#shfmt"
exit 1
fi
- shfmt -w .
- shfmt -w "{{.SCRIPT_PATH}}"

# Make a temporary file named according to the passed TEMPLATE variable and print the path passed to stdout
# Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/windows-task/Taskfile.yml
Expand Down
73 changes: 27 additions & 46 deletions workflow-templates/assets/check-shell-task/Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,78 +2,59 @@
version: "3"

tasks:
# Parameter variables:
# - SCRIPT_PATH: path of the script to be checked.
# Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml
shell:check:
desc: Check for problems with shell scripts
cmds:
- |
if [[ "{{.SCRIPT_PATH}}" == "" ]]; then
echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable."
echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage"
exit 1
fi
- |
if ! which shellcheck &>/dev/null; then
echo "shellcheck not installed or not in PATH."
echo "Please install: https://github.com/koalaman/shellcheck#installing"
exit 1
fi
- |
# There is something odd about shellcheck that causes the task to always exit on the first fail, despite any
# measures that would prevent this with any other command. So it's necessary to call shellcheck only once with
# the list of script paths as an argument. This could lead to exceeding the maximum command length on Windows if
# the repository contained a large number of scripts, but it's unlikely to happen in reality.
shellcheck \
--format={{default "tty" .SHELLCHECK_FORMAT}} \
$(
# The odd method for escaping . in the regex is required for windows compatibility because mvdan.cc/sh gives
# \ characters special treatment on Windows in an attempt to support them as path separators.
find . \
-type d -name '.git' -prune -or \
-type d -name '.licenses' -prune -or \
-type d -name '__pycache__' -prune -or \
-type d -name 'node_modules' -prune -or \
\( \
-regextype posix-extended \
-regex '.*[.](bash|sh)' -and \
-type f \
\) \
-print
)
"{{.SCRIPT_PATH}}"
# Parameter variables:
# - SCRIPT_PATH: path of the script to be checked.
# Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml
shell:check-mode:
desc: Check for non-executable shell scripts
cmds:
- |
EXIT_STATUS=0
while read -r nonExecutableScriptPath; do
# The while loop always runs once, even if no file was found
if [[ "$nonExecutableScriptPath" == "" ]]; then
continue
fi
echo "::error file=${nonExecutableScriptPath}::non-executable script file: $nonExecutableScriptPath";
EXIT_STATUS=1
done <<<"$(
# The odd approach to escaping `.` in the regex is required for windows compatibility because mvdan.cc/sh
# gives `\` characters special treatment on Windows in an attempt to support them as path separators.
find . \
-type d -name '.git' -prune -or \
-type d -name '.licenses' -prune -or \
-type d -name '__pycache__' -prune -or \
-type d -name 'node_modules' -prune -or \
\( \
-regextype posix-extended \
-regex '.*[.](bash|sh)' -and \
-type f -and \
-not -executable \
-print \
\)
)"
exit $EXIT_STATUS
if [[ "{{.SCRIPT_PATH}}" == "" ]]; then
echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable."
echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage"
exit 1
fi
- |
test -x "{{.SCRIPT_PATH}}"
# Parameter variables:
# - SCRIPT_PATH: path of the script to be formatted.
# Source: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/assets/check-shell-task/Taskfile.yml
shell:format:
desc: Format shell script files
cmds:
- |
if [[ "{{.SCRIPT_PATH}}" == "" ]]; then
echo "Path to script file must be passed to this task via the SCRIPT_PATH taskfile variable."
echo "See: https://github.com/arduino/tooling-project-assets/blob/main/workflow-templates/check-shell-task.md#usage"
exit 1
fi
- |
if ! which shfmt &>/dev/null; then
echo "shfmt not installed or not in PATH. Please install: https://github.com/mvdan/sh#shfmt"
exit 1
fi
- shfmt -w .
- shfmt -w "{{.SCRIPT_PATH}}"
60 changes: 60 additions & 0 deletions workflow-templates/check-shell-task.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,30 @@ Install the [`check-shell-task.yml`](check-shell-task.yml) GitHub Actions workfl

The formatting style defined in `.editorconfig` is the official standardized style to be used in all Arduino tooling projects and should not be modified.

### Configuration

Configure the paths of the shell scripts to be checked as elements in the [job matrices](https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategymatrix) of `check-shell-task.yml` at:

- `jobs.lint.strategy.matrix.script[]`
- `jobs.formatting.strategy.matrix.script[]`
- `jobs.executable.strategy.matrix.script[]`

#### Example:

```yaml
matrix:
script:
- path/to/some-script.sh
- path/to/another-script.sh
```
#### Paths filters
The workflow is configured to be triggered on changes to any files in the repository that have a `.sh` or `.bash` file extension. If the project contains shell scripts without a file extension, the path to those scripts must be added to the following keys in `check-shell-task.yml`:

- `on.push.paths[]`
- `on.pull_request.paths[]`

### Readme badge

Markdown badge:
Expand Down Expand Up @@ -64,3 +88,39 @@ On every push or pull request that modifies one of the shell scripts in the repo
- Runs [`shfmt`](https://github.com/mvdan/sh) to check formatting.
- Checks for forgotten executable script file permissions.
```

## Usage

In addition to the automated checks provided by the GitHub Actions workflow, the tasks can be ran locally.

### Prerequisites

The following development tools must be available in your local environment:

- [**ShellCheck**](https://github.com/koalaman/shellcheck#installing) - shell script static analysis tool.
- [**shfmt**](https://github.com/mvdan/sh#shfmt) - shell script formatting tool.
- [**Task**](https://taskfile.dev/installation/) task runner tool.

### Run static analysis

```text
task shell:check SCRIPT_PATH="<script path>"
```

(where `<script path>` is the path to the script file)

### Check file permissions

```text
task shell:check-mode SCRIPT_PATH="<script path>"
```

(where `<script path>` is the path to the script file)

### Format script

```text
task shell:format SCRIPT_PATH="<script path>"
```

(where `<script path>` is the path to the script file)
Loading

0 comments on commit 1098496

Please sign in to comment.