search

sclorg / container-common-scripts

Apache License 2.0
20 stars 45 forks source link

ci(lint): add shell linter - Differential ShellCheck #293

Closed jamacku closed 2 years ago

jamacku commented 2 years ago

Differential ShellCheck is a GitHub action that performs differential ShellCheck scans on shell scripts changed via PR and reports results directly in PR. The beauty of differential scans is that you will get reports only about newly added defects.

Since this repository has many shell scripts, I think you might find some value in having a shell linter. Differential ShellCheck action is able to produce reports in SARIF format. GitHub understands this format and is able to display it nicely as a PR comment, and on the Files Changed tab, please see below.

image

image

Documentation is available at @redhat-plumbers-in-action/differential-shellcheck. Let me know If you are missing some feature or setting. I'm always happy to extend functionality.

Note: I have also removed .travis.yml. It seems it is no longer used.

jamacku commented 2 years ago

It might be helpful for contributors that would forget to run make shellcheck locally (as noted in sclorg/container-common-scripts#277), and they would still get notification about newly introduced shellcheck defects. And when you get to review, defects might be already addressed.

zmiklank commented 2 years ago

Thank you for the contribution. We already talked about integrating the differential shell check, see https://github.com/sclorg/container-common-scripts/issues/278 (but more people knew about it, not just me :)).

However, we already have a shellcheck running on demand with other tests on PR as part of SanityTest GitHub Action (see e.g., here).

I see a big advantage of checking only PR changes when using differential-shellcheck. This would be useful for example in the PR you mentioned above (https://github.com/sclorg/container-common-scripts/pull/277#issuecomment-1210497736).

I personally am for removing our existing shellcheck GitHub action and start using this one. One more think to consider, is that out existing sanity-check GitHub Action also executes a pre-commit-check, which should not be removed.

@phracek WDYT?

I will also try to run our tests, so that I can see how the results will be displayed, when the shellcheck results are already present. [test-all]

phracek commented 2 years ago

I totally agree with @zmiklank From my point of view it looks good to me. I would prefer to add this one check but let's not remove our spellcheck.

@jamacku Can you please rebase this pull request against master?

jamacku commented 2 years ago

@jamacku Can you please rebase this pull request against master?

Done :+1:

phracek commented 2 years ago

[test-all]

jamacku commented 2 years ago

@zmiklank, @phracek, would you be interested in differential-shellcheck GitHub Action in other repositories with shell scripts?

zmiklank commented 2 years ago

@jamacku I am not aware of any other repository that would need shellcheck running, thanks.