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

Put some pre-commit checks in place #7

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nicholasmhughes
Copy link

Just a suggestion, but putting some code standards in place programmatically with pre-commit will keep the code looking uniform.

There's also a bandit security check that's failing in several sections of the code which you might want to investigate:

>> Issue: [B602:subprocess_popen_with_shell_equals_true] subprocess call with shell=True identified, security issue.
   Severity: High   Confidence: High
   Location: plugins/ECESecurity.py:237
   More Info: https://bandit.readthedocs.io/en/latest/plugins/b602_subprocess_popen_with_shell_equals_true.html

If you accept this, you'll probably want to put a pre-commit GitHub Action in place pretty quickly to ensure contributors are using it.

This is just a starting point. Might be cool to extend with pylint and other things that make sense.

@jonrau-lightspin
Copy link
Contributor

Hey @nicholasmhughes looks like black is turning my precious single quotes into double quotes.

That Bandit check is the same thing that Snyk and CodeQL yell at me about - I will do some testing on setting shell=False - it should still work. I took that subprocess code from another project of ours. I'll likely abandon this PR after that testing as it will remediate the core issues you found. Trivy, Snyk and CodeQL/Dependabot are all running as GHAs to prevent PRs though

@jonrau-lightspin jonrau-lightspin added bug Something isn't working enhancement New feature or request labels Mar 2, 2022
@nicholasmhughes
Copy link
Author

Good (controversial) discussion here on quoting, but scroll to the bottom if you really want single quotes. It's possible to keep them.

Instituting pre-commit checks is a good way to allow contributors to ensure they're adhering to standards before commit and pushing... only to find out that something is failing the pipeline.

@szotrj
Copy link

szotrj commented Jan 15, 2023

I recommend running pylint to cleanup your Python style (PEP 8) (e.g. snake_case vs camelCase) -- didn't want to put you on blast on LinkedIn ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants