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

Add shellcheck CI #7888

Merged
merged 6 commits into from
Oct 8, 2024
Merged

Add shellcheck CI #7888

merged 6 commits into from
Oct 8, 2024

Conversation

sobolevn
Copy link
Contributor

@sobolevn sobolevn commented Oct 3, 2024

Summary

I started learning uv by inspecting the source code.
I've noticed that your shell scripts are very good! Which is rare!

Test Plan

I propose to add shellcheck to the CI.
It is a great tool to help finding bugs and style issues in shell code.

Techincal details:

  • This CI job will only run when any .sh files are changed (or the job definition file)
  • It takes just several seconds even on local machine:
» time shellcheck -S style **/*.sh
shellcheck -S style **/*.sh  0.02s user 0.05s system 61% cpu 0.123 total

But, I understand that build / lint tools are very subjective. So, feel free to close :)

@zanieb
Copy link
Member

zanieb commented Oct 3, 2024

I do really like shellcheck but given it didn't find any actual problems here I'm not sure if it's worth adding to CI?

If we do add it, it'd probably make sense at

with our other lint jobs.

@sobolevn
Copy link
Contributor Author

sobolevn commented Oct 3, 2024

@zanieb this is exactly right time to add a tool, when there are no complaints! 😄
Because in other cases - it would be a much harder case, since it would require a refactoring of existing problems.

I added this a separate file, because I can specify to only run this on *.sh files to save some extra computation, when regular PRs don't touch shell scripts.

@zanieb
Copy link
Member

zanieb commented Oct 3, 2024

How long does it take to run on the full repository? Seconds right? I'm not sure it's worth the time saving — spinning up GitHub Actions runners has significant overhead.

@sobolevn
Copy link
Contributor Author

sobolevn commented Oct 3, 2024

Locally it takes 0.02s user 0.05s system 61% cpu 0.123 total, sure, will change to work in lint: job 👍

Copy link
Contributor

@mkniewallner mkniewallner left a comment

Choose a reason for hiding this comment

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

It would probably be nice to pin a specific shellcheck version to use, per https://github.com/ludeeus/action-shellcheck?tab=readme-ov-file#run-a-specific-version-of-shellcheck. Otherwise if new versions that update or add rules are published, CI could break out of the blue.

By enabling https://docs.renovatebot.com/presets-customManagers/#custommanagersgithubactionsversions preset here and setting something like:

env:
  # renovate: datasource=github-tags depName=koalaman/shellcheck
  SHELLCHECK_VERSION: "v0.10.0"

in the GitHub Actions workflow, it should even be possible to have Renovate regularly update shellcheck version.

@sobolevn
Copy link
Contributor Author

sobolevn commented Oct 3, 2024

Awesome! I didn't know that!

@zanieb zanieb merged commit 5652193 into astral-sh:main Oct 8, 2024
61 checks passed
@zanieb zanieb added the internal A refactor or improvement that is not user-facing label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal A refactor or improvement that is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants