Skip to content

Conversation

@hoffie
Copy link
Member

@hoffie hoffie commented May 20, 2022

Short description of changes
Run shellcheck & shfmt on PRs & branches

  • Uses least privilege approach and does not rely on external actions.
  • Runs on all pushes or pull requests which touch .sh files. Can also be run manually.

Example run:
https://github.com/hoffie/jamulus/runs/6716242088?check_suite_focus=true

CHANGELOG: Scripts: Coding style and static checks for shell scripts are now enforced.

Context: Fixes an issue?
Related: #2474

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Ready.

What is missing until this pull request can be merged?

Reviews.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added this to the Release 3.9.0 milestone May 20, 2022
@hoffie hoffie requested a review from ann0see May 20, 2022 22:29
@ann0see
Copy link
Member

ann0see commented May 21, 2022

Any advantages/disadvantages to put this into the clang format run?

@hoffie
Copy link
Member Author

hoffie commented May 21, 2022

Do you mean combining the workflows?
That would be possible, but I don't think we could restrict this workflow to just shell files in that case (meaning that it will always run, something which we are trying to reduce...? :)).

@ann0see
Copy link
Member

ann0see commented May 21, 2022

Spinning up a runner also costs. Maybe even more?

@pljones
Copy link
Collaborator

pljones commented May 21, 2022

Does anyone manually run the clang-format check? Would they be expecting it to start worrying about scripts?

Conversely, would anyone want to only run the script check without the full clang-format check?

Given how quick running "make clang_format" is, I'd expect spinning up the builder to be the most expensive part of the process in terms of elapse time, if nothing else. On the whole, I'm tending towards just bundling it all together. I'd keep separate "make" options, though.

@ann0see
Copy link
Member

ann0see commented May 21, 2022

Yeah. Probably putting it together is cleaner and sustainable?

@ann0see
Copy link
Member

ann0see commented May 27, 2022

@hoffie I think we should put it into the clang format file and run it only if there are changes to cpp, mm, sh, h, ... files.

@hoffie hoffie force-pushed the ci-check-shell-scripts branch 2 times, most recently from 4fe479e to a8db77e Compare June 2, 2022 21:09
@hoffie hoffie force-pushed the ci-check-shell-scripts branch from a8db77e to b639cb0 Compare June 2, 2022 21:10
@hoffie
Copy link
Member Author

hoffie commented Jun 2, 2022

@hoffie I think we should put it into the clang format file and run it only if there are changes to cpp, mm, sh, h, ... files.

Done. It's a bit unnatural to do this in a single job, but splitting into multiple jobs (even as part of the same workflow) would probably require fresh runners so it wouldn't save us anything.

@ann0see ann0see merged commit 784dec7 into jamulussoftware:master Jun 5, 2022
@hoffie hoffie deleted the ci-check-shell-scripts branch August 20, 2022 08:49
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.

3 participants