89luca89
Add another Shell linter - Differential-ShellCheck
Differential ShellCheck is a GitHub action that performs differential ShellCheck scans on shell scripts changed via PR and reports results directly in PR.
I saw that your scripts are in great shape and that you already have shell linters in place, but I still think that you might find differential-shellcheck action useful. It is able to produce reports in SARIF format, GitHub understands this format and is able to display it nicely as PR comment and on the Files Changed tab, please see below.
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.
This is very cool thanks!
Is there a way we can keep in sync the options from the normal shellcheck run and this?
Specifically:
-s sh -a -o all -Sstyle -Calways -x -e SC2310,SC2311,SC2312
This is very cool thanks!
I'm glad you like it! :+1:
Is there a way we can keep in sync the options from the normal shellcheck run and this?
Specifically:
-s sh -a -o all -Sstyle -Calways -x -e SC2310,SC2311,SC2312
differential-shellcheck supports configuration via the .shellcheckrc configuration file supported by ShellCheck itself. And it should be possible to set most of the relevant options via this file.
-s sh-.shellcheckrc~shell=sh-a- Currently not possible, but because differential-shellcheck reports only new defects, it's not useful IMHO.-o all-.shellcheckrc~enable=all-Sstyle- Currently not possible - But it is default as far as I'm aware. Also, there is a plan to implement support for this option: https://github.com/redhat-plumbers-in-action/differential-shellcheck/issues/75-Calways- Currently not possible - But it is not relevant, since the output will be displayed in GitHub UI.-x-.shellcheckrc~external-sources=true; now enabled by default - https://github.com/redhat-plumbers-in-action/differential-shellcheck/pull/91-e SC2310,SC2311,SC2312-.shellcheckrc~disable=SC2310,disable=SC2311,disable=SC2312
The advantage of using .shellcheckrc is that when you run shellcheck locally in your IDE or manually, you will get the same results as in GitHub actions.
If you would like to see more options supported feel free to submit an issue.
Differential ShellCheck now allows to define minimal defect severity to be reported - https://github.com/redhat-plumbers-in-action/differential-shellcheck#severity
The default value is set to style.
Very nice thanks!
Sorry If I'm slow on this one, but I'd like to study this a bit and do some tests :smile:
I've unified the flags between the lint and the differential action, I'll use this PR to test the action with a bit of errors to see how it behaves, then I'm pretty sure we can merge :smile:
Beautiful! :smile:
@jamacku but this is missing something, maybe some configuration is missing?
It's reporting the style error but not the others. This is the output of shellcheck from terminal (same rc file)
It missed a Warning and and Info Do I need to specify the severities I want as a list? From docs it seems it should behave like shellcheck (setting the minimum value)
Thanks!
@89luca89, missing comments are weird, but I think there is nothing I can do about it. I already saw it (but rarely).
For context, differential-shellcheck will generate a report in SARIF format and then upload it to GitHub. GitHub parses the report and shows warnings in Files changed tab and creates comments.
When you inspect Files changed tab, you can see that there are indeed three defects reported.
Also, when you look at the action run, you can see that it detected three defects correctly.
What concerns me is the last report pointing to ./distrobox-create:619:9 when an error is on line 616. When looking at your current shellcheck action, it reported the same thing. The only explanation I came up with is some race conditions during the checkout part (maybe some force push). That could also cause missing comments. I would try to rerun the test suite manually.
Yea ran it manually again but the comments didn't work :shrug: About the line it also stayed the same, still detected 619 instead of 616
I have rebased PR on top of the current main. Let's see if it makes any difference.
I have rebased PR on top of the current main. Let's see if it makes any difference.
Seems like the line number is fixed now :+1: Just missing the comments
I'm not sure what caused the missing comments. GitHub's processing of SARIF is black-box. :facepalm: It recently happened on https://github.com/cea-hpc/modules/pull/471/files#diff-41fa2dac924cdeb2ce53a18b52ebe93233d4ef3b34440ddb03ce176bb2aa3360 as well.
@89luca89 I would guess that GitHub doesn't check if there is a comment for a given defect (that's why there were no comments created after rebase), but when you add a new defect, it shows the comment correctly.
@89luca89 I would guess that GitHub doesn't check if there is a comment for a given defect (that's why there were no comments created after rebase), but when you add a new defect, it shows the comment correctly.
Cool seems ok Last question, is there something that can indicate the user that they need to check the "Files Changed" page for a full list of warnings? That would work around the missing comments and point the user to where the warnings are displayed
Cool seems ok Last question, is there something that can indicate the user that they need to check the "Files Changed" page for a full list of warnings? That would work around the missing comments and point the user to where the warnings are displayed
No, there is no such feature at the moment. I could add a comment mentioning that, but it would be redundant when there would be comments from GitHub, so it would require some complex logic that would try to finger out if GitHub was able to display comments or not. Also, it would be really race depending on how fast GitHub would be able to parse SARIF and show results. And to me, this seems like too much work for little, given that this rarely happens from what I saw.
One indication would lead users to the Files changed tab. differential-shellcheck supports Job summary, and when the user clicks on Errors / Warnings / Notes it will navigate them to the tab. That requires users to try to inspect failing CI, but that is also the case in the current CI setup.
Understandable, anyway seems to work fine, we'll see if there are any problems in the future :smile: Thanks a lot for the contribution!
Understandable, anyway seems to work fine, we'll see if there are any problems in the future smile Thanks a lot for the contribution!
Feel free to ping me if there is some issue or feature request.