Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add shell linter #59

Merged
merged 2 commits into from
Oct 27, 2022
Merged

add shell linter #59

merged 2 commits into from
Oct 27, 2022

Conversation

teknoraver
Copy link
Contributor

@teknoraver teknoraver commented Oct 25, 2022 •
edited
Loading

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

danielocfb
danielocfb requested changes Oct 25, 2022
View reviewed changes
Copy link
Collaborator

@danielocfb danielocfb left a 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?

.github/workflows/lint.yaml Outdated Show resolved Hide resolved
@teknoraver teknoraver force-pushed the shellcheck branch 2 times, most recently from 155befe to 799ad0d Compare October 25, 2022 20:30
@teknoraver
Copy link
Contributor Author

oops, my bad, I'll fix the subject

@teknoraver teknoraver changed the title add shell liter add shell linter Oct 26, 2022
@teknoraver
Copy link
Contributor Author

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
Copy link
Collaborator

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
shellcheck: fix errors
a1c4200 
Signed-off-by: Matteo Croce <teknoraver@meta.com>
@teknoraver teknoraver force-pushed the shellcheck branch 2 times, most recently from 465c1b7 to f901b79 Compare October 26, 2022 14:08
@teknoraver
Copy link
Contributor Author

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

@danielocfb danielocfb self-requested a review October 26, 2022 16:45
danielocfb
danielocfb reviewed Oct 26, 2022
View reviewed changes
Copy link
Collaborator

@danielocfb danielocfb left a 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?

rootfs/mkrootfs_tweak.sh Outdated Show resolved Hide resolved
rootfs/mkrootfs_debian.sh Outdated Show resolved Hide resolved
prepare-rootfs/run.sh Outdated Show resolved Hide resolved
helpers.sh Outdated Show resolved Hide resolved
get-linux-source/checkout_latest_kernel.sh Outdated Show resolved Hide resolved
download-vmlinux/run.sh Outdated Show resolved Hide resolved
patch-kernel/patch_kernel.sh Outdated Show resolved Hide resolved
@teknoraver
add shell linter shellcheck
e63022f 
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 <teknoraver@meta.com>
@teknoraver
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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 ?

@teknoraver
Copy link
Contributor Author

teknoraver commented Oct 27, 2022 •
edited
Loading

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:
ludeeus/action-shellcheck#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:
ludeeus/action-shellcheck#70

@teknoraver
Copy link
Contributor Author

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 danielocfb merged commit 4dfc73d into libbpf:master Oct 27, 2022
@danielocfb
Copy link
Collaborator

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

@teknoraver teknoraver deleted the shellcheck branch October 27, 2022 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielocfb danielocfb danielocfb requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@teknoraver @danielocfb