Skip to content

Add actionlint as a pre-commit hook (with shellcheck integration) #15021

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Add actionlint as a pre-commit hook (with shellcheck integration) #15021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad00186
Add `actionlint` as a pre-commit hook (with shellcheck integration)
AlexWaygood Dec 16, 2024
9eaca50
use quotes on the outer value
AlexWaygood Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/actionlint.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Configuration for the actionlint tool, which we run via pre-commit
# to verify the correctness of the syntax in our GitHub Actions workflows.

self-hosted-runner:
# Various runners we use that aren't recognized out-of-the-box by actionlint:
labels:
- depot-ubuntu-latest-8
- depot-ubuntu-22.04-16
- windows-latest-xlarge
8 changes: 4 additions & 4 deletions .github/workflows/build-binaries.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ jobs:
args: --out dist
- name: "Test sdist"
run: |
pip install dist/${PACKAGE_NAME}-*.tar.gz --force-reinstall
pip install dist/"${PACKAGE_NAME}"-*.tar.gz --force-reinstall
"${MODULE_NAME}" --help
python -m "${MODULE_NAME}" --help
- name: "Upload sdist"
Expand Down Expand Up @@ -125,7 +125,7 @@ jobs:
args: --release --locked --out dist
- name: "Test wheel - aarch64"
run: |
pip install dist/${PACKAGE_NAME}-*.whl --force-reinstall
pip install dist/"${PACKAGE_NAME}"-*.whl --force-reinstall
ruff --help
python -m ruff --help
- name: "Upload wheels"
Expand Down Expand Up @@ -186,7 +186,7 @@ jobs:
if: ${{ !startsWith(matrix.platform.target, 'aarch64') }}
shell: bash
run: |
python -m pip install dist/${PACKAGE_NAME}-*.whl --force-reinstall
python -m pip install dist/"${PACKAGE_NAME}"-*.whl --force-reinstall
"${MODULE_NAME}" --help
python -m "${MODULE_NAME}" --help
- name: "Upload wheels"
Expand Down Expand Up @@ -236,7 +236,7 @@ jobs:
- name: "Test wheel"
if: ${{ startsWith(matrix.target, 'x86_64') }}
run: |
pip install dist/${PACKAGE_NAME}-*.whl --force-reinstall
pip install dist/"${PACKAGE_NAME}"-*.whl --force-reinstall
"${MODULE_NAME}" --help
python -m "${MODULE_NAME}" --help
- name: "Upload wheels"
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/build-docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ jobs:
# The printf will expand the base image with the `<RUFF_BASE_IMG>@sha256:<sha256> ...` for each sha256 in the directory
# The final command becomes `docker buildx imagetools create -t tag1 -t tag2 ... <RUFF_BASE_IMG>@sha256:<sha256_1> <RUFF_BASE_IMG>@sha256:<sha256_2> ...`
run: |
# shellcheck disable=SC2046
docker buildx imagetools create \
$(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \
$(printf "${RUFF_BASE_IMG}@sha256:%s " *)
Expand Down Expand Up @@ -286,6 +287,8 @@ jobs:
# The final command becomes `docker buildx imagetools create -t tag1 -t tag2 ... <RUFF_BASE_IMG>@sha256:<sha256_1> <RUFF_BASE_IMG>@sha256:<sha256_2> ...`
run: |
readarray -t lines <<< "$DOCKER_METADATA_OUTPUT_ANNOTATIONS"; annotations=(); for line in "${lines[@]}"; do annotations+=(--annotation "$line"); done

# shellcheck disable=SC2046
docker buildx imagetools create \
"${annotations[@]}" \
$(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \
Comment on lines +290 to 294
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might actually be ok to quote but it's ok for now to disable this as we might need to test this a bit to verify.

But, the other one is probably correct to ignore because it contains glob * character.

Expand Down
20 changes: 10 additions & 10 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ jobs:

ruff-ecosystem check ./ruff ${{ steps.ruff-target.outputs.download-path }}/ruff --cache ./checkouts --output-format markdown | tee ecosystem-result-check-stable

cat ecosystem-result-check-stable > $GITHUB_STEP_SUMMARY
cat ecosystem-result-check-stable > "$GITHUB_STEP_SUMMARY"
echo "### Linter (stable)" > ecosystem-result
cat ecosystem-result-check-stable >> ecosystem-result
echo "" >> ecosystem-result
Expand All @@ -454,7 +454,7 @@ jobs:

ruff-ecosystem check ./ruff ${{ steps.ruff-target.outputs.download-path }}/ruff --cache ./checkouts --output-format markdown --force-preview | tee ecosystem-result-check-preview

cat ecosystem-result-check-preview > $GITHUB_STEP_SUMMARY
cat ecosystem-result-check-preview > "$GITHUB_STEP_SUMMARY"
echo "### Linter (preview)" >> ecosystem-result
cat ecosystem-result-check-preview >> ecosystem-result
echo "" >> ecosystem-result
Expand All @@ -470,7 +470,7 @@ jobs:

ruff-ecosystem format ./ruff ${{ steps.ruff-target.outputs.download-path }}/ruff --cache ./checkouts --output-format markdown | tee ecosystem-result-format-stable

cat ecosystem-result-format-stable > $GITHUB_STEP_SUMMARY
cat ecosystem-result-format-stable > "$GITHUB_STEP_SUMMARY"
echo "### Formatter (stable)" >> ecosystem-result
cat ecosystem-result-format-stable >> ecosystem-result
echo "" >> ecosystem-result
Expand All @@ -486,7 +486,7 @@ jobs:

ruff-ecosystem format ./ruff ${{ steps.ruff-target.outputs.download-path }}/ruff --cache ./checkouts --output-format markdown --force-preview | tee ecosystem-result-format-preview

cat ecosystem-result-format-preview > $GITHUB_STEP_SUMMARY
cat ecosystem-result-format-preview > "$GITHUB_STEP_SUMMARY"
echo "### Formatter (preview)" >> ecosystem-result
cat ecosystem-result-format-preview >> ecosystem-result
echo "" >> ecosystem-result
Expand Down Expand Up @@ -570,13 +570,13 @@ jobs:
key: pre-commit-${{ hashFiles('.pre-commit-config.yaml') }}
- name: "Run pre-commit"
run: |
echo '```console' > $GITHUB_STEP_SUMMARY
echo '```console' > "$GITHUB_STEP_SUMMARY"
# Enable color output for pre-commit and remove it for the summary
SKIP=cargo-fmt,clippy,dev-generate-all pre-commit run --all-files --show-diff-on-failure --color=always | \
tee >(sed -E 's/\x1B\[([0-9]{1,2}(;[0-9]{1,2})*)?[mGK]//g' >> $GITHUB_STEP_SUMMARY) >&1
exit_code=${PIPESTATUS[0]}
echo '```' >> $GITHUB_STEP_SUMMARY
exit $exit_code
tee >(sed -E 's/\x1B\[([0-9]{1,2}(;[0-9]{1,2})*)?[mGK]//g' >> "$GITHUB_STEP_SUMMARY") >&1
exit_code="${PIPESTATUS[0]}"
echo '```' >> "$GITHUB_STEP_SUMMARY"
exit "$exit_code"

docs:
name: "mkdocs"
Expand Down Expand Up @@ -637,7 +637,7 @@ jobs:
- name: "Run checks"
run: scripts/formatter_ecosystem_checks.sh
- name: "Github step summary"
run: cat target/formatter-ecosystem/stats.txt > $GITHUB_STEP_SUMMARY
run: cat target/formatter-ecosystem/stats.txt > "$GITHUB_STEP_SUMMARY"
- name: "Remove checkouts from cache"
run: rm -r target/formatter-ecosystem

Expand Down
1 change: 1 addition & 0 deletions .github/workflows/daily_fuzz.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ jobs:
run: cargo build --locked
- name: Fuzz
run: |
# shellcheck disable=SC2046
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be ok to quote this:

"$(shuf ...)"

Although I don't really have a linux machine to test. I'd be fine to ignore this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked it on my Mac, and the command copied-and-pasted into my shell works as it currently is on main, but adding the quoting breaks it. It's possible it might be different on Linux, but it's also just quite nice to use commands in our GitHub workflows that I can test locally

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, let's keep it disabled then. Thanks for testing it.

(
uvx \
--python=3.12 \
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/pr-comment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ jobs:
run: |
if [[ -f pr-number ]]
then
echo "pr-number=$(<pr-number)" >> $GITHUB_OUTPUT
echo "pr-number=$(<pr-number)" >> "$GITHUB_OUTPUT"
fi

- uses: dawidd6/action-download-artifact@v7
Expand Down Expand Up @@ -66,9 +66,9 @@ jobs:
cat pr/ecosystem/ecosystem-result >> comment.txt
echo "" >> comment.txt

echo 'comment<<EOF' >> $GITHUB_OUTPUT
cat comment.txt >> $GITHUB_OUTPUT
echo 'EOF' >> $GITHUB_OUTPUT
echo 'comment<<EOF' >> "$GITHUB_OUTPUT"
cat comment.txt >> "$GITHUB_OUTPUT"
echo 'EOF' >> "$GITHUB_OUTPUT"

- name: Find existing comment
uses: peter-evans/find-comment@v3
Expand Down
20 changes: 11 additions & 9 deletions .github/workflows/publish-docs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ jobs:
# Use version as display name for now
display_name="$version"

echo "version=$version" >> $GITHUB_ENV
echo "display_name=$display_name" >> $GITHUB_ENV
echo "version=$version" >> "$GITHUB_ENV"
echo "display_name=$display_name" >> "$GITHUB_ENV"

- name: "Set branch name"
run: |
Expand All @@ -55,8 +55,8 @@ jobs:
# characters disallowed in git branch names with hyphens
branch_display_name="$(echo "${display_name}" | tr -c '[:alnum:]._' '-' | tr -s '-')"

echo "branch_name=update-docs-$branch_display_name-$timestamp" >> $GITHUB_ENV
echo "timestamp=$timestamp" >> $GITHUB_ENV
echo "branch_name=update-docs-$branch_display_name-$timestamp" >> "$GITHUB_ENV"
echo "timestamp=$timestamp" >> "$GITHUB_ENV"

- name: "Add SSH key"
if: ${{ env.MKDOCS_INSIDERS_SSH_KEY_EXISTS == 'true' }}
Expand Down Expand Up @@ -112,7 +112,7 @@ jobs:
GITHUB_TOKEN: ${{ secrets.ASTRAL_DOCS_PAT }}
run: |
# set the PR title
pull_request_title="Update ruff documentation for "${display_name}""
pull_request_title="Update ruff documentation for ${display_name}"

# Delete any existing pull requests that are open for this version
# by checking against pull_request_title because the new PR will
Expand All @@ -124,10 +124,12 @@ jobs:
git push origin "${branch_name}"

# create the PR
gh pr create --base main --head "${branch_name}" \
--title "$pull_request_title" \
--body "Automated documentation update for "${display_name}"" \
--label "documentation"
gh pr create \
--base=main \
--head="${branch_name}" \
--title="${pull_request_title}" \
--body="Automated documentation update for ${display_name}" \
--label="documentation"

- name: "Merge Pull Request"
if: ${{ inputs.plan != '' && !fromJson(inputs.plan).announcement_tag_is_implicit }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/sync_typeshed.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ jobs:
run: |
cd ruff
git push --force origin typeshedbot/sync-typeshed
gh pr list --repo $GITHUB_REPOSITORY --head typeshedbot/sync-typeshed --json id --jq length | grep 1 && exit 0 # exit if there is existing pr
gh pr list --repo "$GITHUB_REPOSITORY" --head typeshedbot/sync-typeshed --json id --jq length | grep 1 && exit 0 # exit if there is existing pr
gh pr create --title "Sync vendored typeshed stubs" --body "Close and reopen this PR to trigger CI" --label "internal"

create-issue-on-failure:
Expand Down
18 changes: 18 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,23 @@ repos:
hooks:
- id: check-github-workflows

# `actionlint` hook, for verifying correct syntax in GitHub Actions workflows.
# Some additional configuration for `actionlint` can be found in `.github/actionlint.yaml`.
- repo: https://github.com/rhysd/actionlint
rev: v1.7.4
hooks:
- id: actionlint
# `release.yml` is autogenerated by `dist`; issues need to be fixed there
# (https://opensource.axo.dev/cargo-dist/)
exclude: .github/workflows/release.yml
args:
- "-ignore=SC2129" # ignorable stylistic lint from shellcheck
- "-ignore=SC2016" # another shellcheck lint: seems to have false positives?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide the case where it's giving false positive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the report:

Lint GitHub Actions workflow files.......................................Failed
- hook id: actionlint
- exit code: 1

.github/workflows/publish-docs.yml:113:9: shellcheck reported issue in this script: SC2016:info:7:43: Expressions don't expand in single quotes, use double quotes for that [shellcheck]
    |
113 |         run: |
    |         ^~~~
.github/workflows/pr-comment.yaml:52:9: shellcheck reported issue in this script: SC2016:info:13:6: Expressions don't expand in single quotes, use double quotes for that [shellcheck]
   |
52 |         run: |
   |         ^~~~

they might be fixable? Not sure. I don't think there's an actual bug on those lines anyway.

We could just put inline ignore comments on those lines? But that specific check didn't seem to actually flag any real issues in our shell scripts

Copy link
Member

@dhruvmanila dhruvmanila Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I think the one in publish-docs is a false positive because the pull_request_title needs to be quoted as the title contains multiple words separated by whitespace:

$ gh pr list --state open --json title --jq ".[] | select(.title == ${pull_request_title}) | .number"
failed to parse jq expression (line 1, column 35)
    .[] | select(.title == [red-knot] Statically known branches) | .number
                                      ^  unexpected token "Statically"

Let's ignore this for now as we know it's already working.

additional_dependencies:
# actionlint has a shellcheck integration which extracts shell scripts in `run:` steps from GitHub Actions
# and checks these with shellcheck. This is arguably its most useful feature,
# but the integration only works if shellcheck is installed
- "github.com/wasilibs/go-shellcheck/cmd/[email protected]"

ci:
skip: [cargo-fmt, dev-generate-all]
Loading