search

libbpf / ci

BPF CI
Other
10 stars 23 forks source link

add shell linter #59

Closed teknoraver closed 1 year ago

teknoraver commented 1 year ago

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.

~/src/libbpf-ci$ find -type f -name '*.sh' |xargs shellcheck -S error
~/src/libbpf-ci$ echo $?
0
teknoraver commented 1 year ago

oops, my bad, I'll fix the subject

teknoraver commented 1 year ago

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?

I guess that this is the most used one. Do you suggest another one?

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?

The check works, but finds tons of warnings. I'm addressing all of them in my private repo, so to avoid spamming this one.

danielocfb commented 1 year ago

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?

I guess that this is the most used one. Do you suggest another one?

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.

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?

The check works, but finds tons of warnings. I'm addressing all of them in my private repo, so to avoid spamming this one.

Either is fine, thanks!

teknoraver commented 1 year ago

This is a sample run of the action on my fork: https://github.com/teknoraver/libbpf-ci/actions/runs/3329735501/jobs/5507322095

teknoraver commented 1 year ago

I refreshed a PR with a smaller one which only fixes the severe errors. We'll take care of the many warnings later, along with other improvements.

danielocfb commented 1 year ago

Here is a vmtest CI run: https://github.com/kernel-patches/bpf/actions/runs/3339312442 Here libbpf: https://github.com/danielocfb/libbpf/actions/runs/3339319849

danielocfb commented 1 year ago

The pull request causes a new deprecation warning to show up: https://github.com/libbpf/ci/actions/runs/3339058943. We just removed those: https://github.com/kernel-patches/vmtest/issues/142. It's not blocking, but would you mind trying to get the warning removed, @teknoraver ?

teknoraver commented 1 year ago

This needs to be fixed in the action itself: https://github.com/ludeeus/action-shellcheck/blob/master/action.yaml#L213-L214

This has been already reported to the action author: https://github.com/ludeeus/action-shellcheck/issues/69

I see that there is a PR already for this, so I hope it will disappear after the merge, as I'm targeting master: https://github.com/ludeeus/action-shellcheck/pull/70

teknoraver commented 1 year ago

BTW according to GitHub, this deprecation was added last week, and deprecated commands will be available until 31st May 2023:

https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

Let's hope that the action will be fixed in the meantime!

danielocfb commented 1 year ago

CI doesn't seem to have any objections. Neither do I. Thanks!