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.

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 shellcheck action to catch script errors #4533

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

openoms
Copy link
Collaborator

@openoms openoms commented Apr 7, 2024

will need to fix or ignore errors

Can be run locally with act https://github.com/nektos/act :

gh extension install https://github.com/nektos/gh-act
  • run this action
gh act -W '.github/workflows/test-shellcheck.yml'

@openoms openoms requested a review from rootzoll as a code owner April 7, 2024 06:53
@openoms openoms force-pushed the add-shellcheck-action-to-catch-script-errors branch from d486eaa to baa8dd7 Compare April 7, 2024 12:37
@rootzoll
Copy link
Collaborator

rootzoll commented Apr 7, 2024

Because this might force a lot of small changes - also in the sd card build area, lets target that change for v1.12.0

@openoms openoms force-pushed the add-shellcheck-action-to-catch-script-errors branch from baa8dd7 to 40ecd60 Compare April 7, 2024 14:04
@openoms
Copy link
Collaborator Author

openoms commented Apr 7, 2024

@rootzoll d52f15c and 68b478d are catching errors which affect the run of the scripts already.

Making these changes allows us to use shellcheck more rigorously so there are no new errors are introduced.

If there are false positives espiacally around using $@ vs "$@" we can ignore those lines explicitly.

@openoms
Copy link
Collaborator Author

openoms commented Apr 7, 2024

Disabled checking the lines which are working correctly, but causing shellcheck errors. The rest are actual bugs.

@openoms
Copy link
Collaborator Author

openoms commented Apr 8, 2024

@rootzoll should we include d52f15c separately?

@rootzoll
Copy link
Collaborator

rootzoll commented Apr 8, 2024

@rootzoll should we include d52f15c separately?

@openoms cherry-picked into dev & v1.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants