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

feat: Add shellcheck and shfmt Pre-Commit Hooks #550

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

Fleshgrinder
Copy link
Contributor

@Fleshgrinder Fleshgrinder commented Feb 21, 2023

This adds shellcheck and shfmt pre-commit hooks. I fixed the most pressing issues that were found, but did not go through the existing scripts otherwise. We are going to need more shell scripts to automate other things, this is why it is a good idea to add these tools to our verification chain.

This also changes the GHA lint workflow to use the official pre-commit action, which also makes use of caching, and thus should be faster. The pre-commit action uses the --color=always option, and that option results in the action hanging forever if paired with local hooks, which we have.

@Fleshgrinder Fleshgrinder requested review from a team as code owners February 21, 2023 10:28
@Fleshgrinder Fleshgrinder self-assigned this Feb 21, 2023
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 21, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7e403d0
Status: ✅  Deploy successful!
Preview URL: https://32e76730.karapace.pages.dev
Branch Preview URL: https://fleshgrinder-shellcheck.karapace.pages.dev

View logs

@Fleshgrinder Fleshgrinder force-pushed the fleshgrinder/shellcheck branch 3 times, most recently from 876f9c1 to 78d4fc0 Compare February 21, 2023 10:40
@@ -6,9 +6,6 @@ pdbpp==0.10.2
psutil==5.9.0
requests==2.27.1

# linting
pre-commit>=2.2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are currently managing this version in two places – here and in the .pre-commit-config.yaml file – and they already diverged. There is no reason to include it here, users who wish to use it can install it on their own (in any way they like), and GitHub Actions uses the pre-commit action now which is versioned separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to remove the action, because it is broken with local hooks, and I had to add filelock as direct dependency, because it was used indirectly. Still, the sentiment towards having this dependency stays the same on my side.

@Fleshgrinder Fleshgrinder force-pushed the fleshgrinder/shellcheck branch 2 times, most recently from 0164777 to f970c38 Compare February 21, 2023 10:58
This adds `shellcheck` and `shfmt` pre-commit hooks. I fixed the most pressing
issues that were found, but did not go through the existing scripts otherwise.
We are going to need more shell scripts to automate other things, this is why it
is a good idea to add these tools to our verification chain.

~This also changes the GHA lint workflow to use the official pre-commit action,
which also makes use of caching, and thus should be faster.~ The pre-commit
action uses the `--color=always` option, and that option results in the action
hanging forever if paired with local hooks, which we have.
@jjaakola-aiven jjaakola-aiven merged commit 5f64f7b into main Feb 22, 2023
@jjaakola-aiven jjaakola-aiven deleted the fleshgrinder/shellcheck branch February 22, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants