-
Notifications
You must be signed in to change notification settings - Fork 25
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 shell linter #59
add shell linter #59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code seems fine to me. Thanks!
Would you mind fixing the typo in the subject ("liter") and write a bit more of a commit message? E.g., why did you choose to use ludeeus/action-shellcheck
and not any other variant?
Lastly, given that you created a pull request I assume you verified that the check works and reported no issues. Can you please provide a pointer to a sample run as well?
oops, my bad, I'll fix the subject |
I guess that this is the most used one. Do you suggest another one?
The check works, but finds tons of warnings. |
I am saying that the reasoning is something that most likely should be contained in the commit message, so that in the future somebody can understand why a decision was made.
Either is fine, thanks! |
Signed-off-by: Matteo Croce <[email protected]>
This is a sample run of the action on my fork: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Few comments.
Disable style suggestions SC1091, SC2181 and SC2001 because they are not
worth fixing in this repository.
Can you please explain what they represent?
This adds a GitHub action which runs a shell linter on all the .sh files. ludeeus/action-shellcheck is just an action which relies on https://github.com/koalaman/shellcheck which is a well known linter. Fix only serious errors by now, we'll take care of warnings later. ~/src/libbpf-ci$ find -type f -name '*.sh' |xargs shellcheck -S error ~/src/libbpf-ci$ echo $? 0 Signed-off-by: Matteo Croce <[email protected]>
I refreshed a PR with a smaller one which only fixes the severe errors. |
Here is a |
The pull request causes a new deprecation warning to show up: https://github.com/libbpf/ci/actions/runs/3339058943. We just removed those: kernel-patches/vmtest#142. It's not blocking, but would you mind trying to get the warning removed, @teknoraver ? |
This needs to be fixed in the action itself: This has been already reported to the action author: I see that there is a PR already for this, so I hope it will disappear after the merge, as I'm targeting master: |
BTW according to GitHub, this deprecation was added last week, and deprecated commands will be available until 31st May 2023: Let's hope that the action will be fixed in the meantime! |
CI doesn't seem to have any objections. Neither do I. Thanks! |
This adds a GitHub action which runs a shell linter on all the .sh files.
ludeeus/action-shellcheck is just an action which relies on
https://github.com/koalaman/shellcheck which is a well known linter.