pyupio/safety

Add pre-commit hook

Opened this issue · 9 comments

lucomsky commented
  • safety version: any
  • Python version: any
  • Operating System: any

Description

Add pre-commit hook to safety repository so one can use it directly in pre-commit-config.yaml

---
repos:
  - repo: https://github.com/pyupio/safety.git
    rev: master
    hooks:
      - id: safety
        args: [check]

See https://github.com/rubik/xenon/blob/master/.pre-commit-hooks.yaml for example.

krasnovmv commented

Why pre-commit-hooks-safety is not suitable?

Jesse-Bakker commented

Why pre-commit-hooks-safety is not suitable?

One uses safety to check for known vulnerabilities in dependencies. Using safety from a third-party source kinda defeats the purpose

  • 👍3
  • 🚀1
bagerard commented

I'd like to revive this post. Many of the well known code-quality tools have integrated pre-commit in their own repository (pylint, flake8, bandit, black, isort, etc), an assumption of the pre-commit tool is that you trust the hooks so as Jesse-Bakker said, having an unnecessary dependency is adding security risk.

If you are willing to accept a PR, let me know. More than happy to help on this.

And I'll take the opportunity to thank you for this lib, it's great!

  • 👍8
yeisonvargasf commented

@bagerard we are open to accepting PRs!

bagerard commented

I had a look into this and came to the conclusion that the current CLI of safety is not very "pre-commit friendly". We could integrate a basic config like this one but due to safety current cli, we have to make some assumption on the repository structure or name of the requirements.txt file.

In fact pre-commit hooks work best with a CLI like
some_entry_point file1 file2 file3

and currently safety check expects e.g. -r requirements_1.txt -r requirements_2.txt.

It's a subtle difference but this drastically complicates the integration, this is the reason why Lucas-C uses a custom entry point (a wrapper on top of "safety check" that changes its interface to the one suggested above --> see here.

I think we have 4 options:

  1. modify safety check interface so that it offers a safety check req1.txt req2.txt interface

  2. add a new entry point in safety tailored for the pre-commit hook (basically porting the entrypoint from Lucas-C in this repo)

  3. add a pre-commit config that would be customized for 1 use case, the most widespread (i.e 1 requirements.txt file), if users would need to deviate from that use case, it would be possible but clumsy (as users would need to workaround the assumptions made to fit that use case)

  4. add most basic pre-commit config, which wouldn't work out of the box and require customization to fit the users' needs, customization wouldn't necessarily be clumsy

Discussion
I guess we discard 1) to avoid breaking everyone's code, and I would discard 3) because configuration would be difficult, which leaves us with option 2 and 4. It all depends if you are ok with having another entry point in safety to allow for a smooth pre-commit integration or prefer to keep the integration with minimal invasiveness (i.e only a .pre-commit-hooks.yaml file)

Let me know your preference @yeisonvargasf

  • 👍2
sshishov commented

I know that we have a pre-commit hook for safety, but it is named differently and cannot be used as local repository hook.

Also the current implementation of safety does not allow to use it directly in the pre-commit because of --file flag accepting one file when pre-commit is sending the files as just arguments

Would it be possible to collaborate with @Lucas-C and move the functionality here from https://github.com/Lucas-C/pre-commit-hooks-safety?

@bagerard what is the conclusion here? are we going to add the support?

I would make the point 1 as a solution and release the breaking release. I do not think that it is a big hassle to change the command from safety check -file file1.txt -file file2.txt -f file3.txt into safety check file1.txt file2.txt file3.txt

P.S. I am okay with second point as well... like if we add custom entry point... meaning we have to add either custom command like safety pre-commit-check ... or we have to even add extra binary...

Lucas-C commented

Hi!

I am the maintainer of https://github.com/Lucas-C/pre-commit-hooks-safety
and just discovered this issue 😊

This sounds like a great idea!
You are welcome to take code from my repository to implement this.

I think the main question is: do the maintainers of this repo ( https://github.com/pyupio/safety ) want to maintain this hook?

Jwomers commented
bagerard commented

any update on this @Jwomers ?