CI: scan all Python scripts #92
Conversation
basak-qcom
force-pushed
the
comprehensive-static-checks
branch
3 times, most recently
from
f7c1cf7 to
61f8546
Compare
|
pylint wants the imports from |
Test jobs for commit 61f8546 |
.github/workflows/static-checks.yml
Outdated
| # Run flake8 against all files outside .git/ that `file` reports | ||
| # as text/x-script.python | ||
| find . -path ./.git -prune -o -print0 | \ | ||
| xargs -0n1 sh -c 'test "$(file --brief --mime-type "$1")" = "text/x-script.python" && printf "%s\000" "$1"' -- | \ |
There was a problem hiding this comment.
I find this hard to parse, but I know it's copy-pasted from the other implementation; could we have "search for files with this MIME type" somewhere and use this so that it's at least in a single place and each action can be kept simple?
I kind of regret the simplicity of *.py!
There was a problem hiding this comment.
I'd like to do that too. Where should it go? Under scripts/, to be sourced by each of these jobs perhaps? Then it would look like:
run: |
. scripts/functions
for_each_mime_type text/x-script.python flake8
Would this be acceptable?
There was a problem hiding this comment.
(suggestions for a better name than for_each_mime_type appreciated!)
|
Making this a draft while I test iterations against actual CI. |
basak-qcom
force-pushed
the
comprehensive-static-checks
branch
from
61f8546 to
2b82e91
Compare
We're already using pylint in CI against this repository, but it currently does not cover this file by accident. To fix that, ideally this file would comply with our CI policy on everything being pylint clean. However, it's difficult currently with our GitHub workflow arrangements to ensure that all imports for all scripts are available at the time pylint runs. Since there are only two instances of this in the entire repository, just skip these two specific import checks for now. I would like to move these checks so they run locally as well with "make check", at which point we'd switch the GitHub workflows to doing that instead, and then everything would be aligned including a single place to define development dependencies. However, these dependencies are not currently available as packages, so it seems too much to expect developers to manage at the moment. That's a rabbithole, so for now, to make progress, skipping these for the sake of two ignore directives seems like a reasonable compromise. Signed-off-by: Robie Basak <[email protected]>
Now that commit b1781ef established a pattern to find all shell scripts for linting, do the same for all Python scripts, refactoring the pattern out into a reusable shell function. This way, adding a file anywhere in the repository will be sufficient to ensure it is scanned, instead of hoping that we remember to amend the lint invocations. Signed-off-by: Robie Basak <[email protected]>
basak-qcom
force-pushed
the
comprehensive-static-checks
branch
from
324e04b to
994439b
Compare
Test jobs for commit 994439b |
|
I think this is ready to land now. I've addressed all review points. |
lool
left a comment
lool
left a comment
There was a problem hiding this comment.
Quick thoughts:
-
I think a shell script rather than function would have prevented the need to have special shellcheck excludes, I'm ok with a function though
-
I now realize there are global static analysis efforts across all Qualcomm repos; perhaps it is worth seeing if we could leverage these and sending our contributions if they are not good enough, rather than maintain our own sauce
Perhaps we should have make check cover the code that people are expected to use, including local static analysis, and the Qualcomm workflows scan all code especially workflow related files
See https://github.com/qualcomm/qcom-actions/blob/main/README.md for global workflows – there might be others, I think there's also a GitHub or CodeScan tool running somewhere as well. |
2 participants
Now that commit b1781ef established a pattern to find all shell scripts for linting, do the same for all Python scripts.
This way, adding a file anywhere in the repository will be sufficient to ensure it is scanned, instead of hoping that we remember to amend the lint invocations.