search

rapidsai / deployment

RAPIDS Deployment Documentation
https://docs.rapids.ai/deployment/stable/
9 stars 28 forks source link

run shellcheck in pre-commit checks #401

Closed jameslamb closed 1 month ago

jameslamb commented 1 month ago

Proposes adding shellcheck to the pre-commit checks, to look for issues in shell scripts here.

This is done via shellcheck-py (https://pypi.org/project/shellcheck-py/), a repackaging of shellcheck for PyPI that allows it to be used in pre-commit without needing a separate system installation.

Notes for Reviewers

Right now there are just 2 little shell scripts in this repo: https://github.com/search?q=repo%3Arapidsai%2Fdeployment%20path%3A**%2F*.sh&type=code

Both are used as the entrypoints in bring-your-own-container SageMaker examples. So the additional coverage we get from this check, in the current state of the repo, is pretty small.

But it's lightweight to install and runs fast, and could help to catch issues in those scripts or others we add in the future, so on balance I think it's worth adding.

why is the ruff version in pre-commit changing?

As part of this, I also ran

pre-commit autoupdate

And that pulled in the latest version of ruff. Figured we might as well, while touching that file anyway.

jameslamb commented 1 month ago

I'm surprised there were no shellcheck failures, theres usually at least one nitpick whenever you add a new linter

Yeah same! Should have put it in the description of the PR... I did actually test that this was working by adding something to one of those shell scripts which I knew would trigger the linter. Ran pre-commit run --all-files and saw it report the error I expected. So it is really running shellcheck and finding the right files.