Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

feat: add pre-commit #225

Merged
merged 17 commits into from
Nov 1, 2021
Merged

feat: add pre-commit #225

merged 17 commits into from
Nov 1, 2021

Conversation

kevholmes
Copy link
Contributor

This resolves #217

Below is a sample config file for projects that would like to use this pre-commit hook in their own repo:

# sample .pre-commit-config.yaml for a project that would like to use datree with pre-commit
repos:
-   repo: https://github.com/datreeio/datree
    # rev could also be a release tag
    rev: fbb260a13ba78e3c268768b7b65c293713bce424
    hooks:
      - id: datree-test

This essentially runs datree test --only-k8s-files file1.yml file2.yaml

The files passed into the command are those detected by pre-commit as staged in git.

@eyarz eyarz self-assigned this Oct 22, 2021
@eyarz
Copy link
Member

eyarz commented Oct 22, 2021

Thank you for the PR!

*Disclaimer - I have never used pre-commit (the framework) before.
I created two new files (.pre-commit-config.yaml & .pre-commit-hooks.yaml) in my repo and copied the content from your PR and I followed the instructions from the pre-commit docs:

  1. install pre-commit
  2. install hook - pre-commit install
  3. commit something with a change in a K8s file

when I'm doing it I was getting this error:
image

After installing Go it worked perfectly, but I'm trying to understand why having Go installed on my machine is a pre-requisite for using this hook? is there a workaround because I think many users don't have (and don't want to have) Go installed on their machine.

@eyarz eyarz removed their assignment Oct 25, 2021
Copy link
Member

@eyarz eyarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see above comment

@kevholmes
Copy link
Contributor Author

Thank you for the PR!

*Disclaimer - I have never used pre-commit (the framework) before. I created two new files (.pre-commit-config.yaml & .pre-commit-hooks.yaml) in my repo and copied the content from your PR and I followed the instructions from the pre-commit docs:

  1. install pre-commit
  2. install hook - pre-commit install
  3. commit something with a change in a K8s file

when I'm doing it I was getting this error: image

After installing Go it worked perfectly, but I'm trying to understand why having Go installed on my machine is a pre-requisite for using this hook? is there a workaround because I think many users don't have (and don't want to have) Go installed on their machine.

Hi there @eyarz, Go is required because of how pre-commit works. It uses the repo we're pointing at in the .pre-commit-hooks.yaml file to pull the source at a specific rev then compiles the project. It caches this binary then executes it during pre-commit run ... stage. There'a also an option to use docker containers if you have a container repo somewhere. I could take a look at attempting to use that instead - but Docker is a pre-requisite for that strategy.

Go strategy
Docker image strategy

@eyarz
Copy link
Member

eyarz commented Oct 26, 2021

🤔 interesting.

So I have some follow-up questions:

  1. how pre-commit know to compile the Go project with the same commands that we are using?
  2. can pre-commit install and execute Datree via bash?
  3. is it considered a legit requirement to have Go installed for pre-commit users?

BTW, we have a docker image: https://hub.docker.com/r/datree/datree

@kevholmes
Copy link
Contributor Author

  1. how pre-commit know to compile the Go project with the same commands that we are using?
  2. can pre-commit install and execute Datree via bash?
  3. is it considered a legit requirement to have Go installed for pre-commit users?

BTW, we have a docker image: https://hub.docker.com/r/datree/datree

  1. I'm not sure if it knows the exact commands, but it does its best to figure it out when we provide the language object language: golang then attempts to build for the target environment it was called in.
  2. It could, but you would need to provide the script somewhere in the https://github.com/datreeio/datree repo. I could see that being challenging to potentially write a script that supports a lot of environments - perhaps not the best solution unless you've already got something like this.
  3. Go isn't a pre-requisite for pre-commit in general (python is though.) It is only for pre-commit hooks being called that point to a repo of a Go project.

I could switch this to run from the Docker image instead, then Go isn't required to be installed on the user's sytem, only Docker or an equivalent OCI runtime.

I also noticed https://hub.docker.com/r/datree/datree isn't tagging versions - I think we can use latest but generally in large CI environments most teams I work with like to pin versions of their analysis tools.

@dimabru
Copy link
Contributor

dimabru commented Oct 26, 2021

Hi @kevholmes,
We have a bash install script in our repo: https://github.com/datreeio/datree/blob/main/install.sh
also it can be accessed with curl to https://get.datree.io

Can we integrate it into the pre-commit hook?
We really want to avoid forcing the user to have go in their project to run datree.

Also, we have specific build flags such as cli version - that is auto increment on every deployment. Seems to me that this way users with pre-commit hooks will not know what version they are using. That will make it very hard for us to debug errors.

@dimabru
Copy link
Contributor

dimabru commented Oct 27, 2021

Staging branch is deprecated and will soon be deleted.
Our new default branch in main.
Please change your pr to merge to main.
Thanks!!

@kevholmes kevholmes changed the base branch from staging to main October 27, 2021 13:07
@kevholmes
Copy link
Contributor Author

Hi @kevholmes, We have a bash install script in our repo: https://github.com/datreeio/datree/blob/main/install.sh also it can be accessed with curl to https://get.datree.io

Can we integrate it into the pre-commit hook? We really want to avoid forcing the user to have go in their project to run datree.

Also, we have specific build flags such as cli version - that is auto increment on every deployment. Seems to me that this way users with pre-commit hooks will not know what version they are using. That will make it very hard for us to debug errors.

I will see if we can use this install script somehow. If that fails the docker container is our best bet. I have used a number of other checks that rely on containers like hadolint (written in Haskell) and they work nicely for end-users who likely do not have ghci installed.

@kevholmes
Copy link
Contributor Author

I think it would be good to include the pre-commit hooks seeded into the repo as part of this commit? This way, a code contributor can skip the pre-commit install step after checking out the repo from GitHub.

@kevholmes
Copy link
Contributor Author

kevholmes commented Oct 28, 2021

I just added two other options. One uses a system pre-commit type with a hard-coded entrypoint to call datree binary directly. It requires datree to be installed already via the install.sh script. Unfortunately there is no setup type step for a script or system pre-commit hook to install hook dependencies. This method also short-circuits pre-commit's awesome ability to help be your CI tool package manager and help control versions (like a go.sum or package-lock.json file.)

The other option uses your Dockerhub image (or attempted to). I ran into an issue with the existing dockerhub image - which appears a bit out of date - so I updated the Dockerfile to give you a much smaller image for end-users to run than what was already there. The size is down from ~430mb to ~20 mb for the image.

@kevholmes
Copy link
Contributor Author

From my perspective the best way to do this is to utilize the docker_image approach that I have included here, while tidying up your Dockerfile using my recommended changes or something similar, and adding release tags to your Dockerhub image commits (which is a best practice even if not using Docker images for pre-commit.) This way a user could specify a tag like datree/datree:v1.42.0 in their .pre-commit-config.yaml file and pin to a specific version for a consistent experience or run :latest for bleeding-edge. Using the Dockerfile I've included makes it a quick 20 MB download for the built image so users are up and running quickly. It still requires a dependency, but that really isn't uncommon when using pre-commit, and allows users to specify and know what version of datree they are running for analysis.

Thoughts @eyarz?

@kevholmes kevholmes requested a review from eyarz October 28, 2021 15:42
Copy link
Contributor Author

@kevholmes kevholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a few other pre-commit methods for the team to review.

@eyarz
Copy link
Member

eyarz commented Oct 28, 2021

@kevholmes, I agree. "latest" is a good practice as an image tag, so I will also open an issue to fix that.
Thank you for making the changes, we will review it as soon as possible.

@eyarz eyarz requested a review from a team October 31, 2021 09:02
Copy link
Contributor

@alexfedin alexfedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevholmes gave you some minor comments, once fixed we can merge.

I got one question, when we will add documentation for the pre-commit hooks usage, do you know what is considered the best practice for the value in the rev key assuming we always want to use the latest version?

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved
scripts/release.sh Outdated Show resolved Hide resolved
@kevholmes
Copy link
Contributor Author

@MentalBrake In regards to best practices for rev key: Unfortunately it looks like the docker_image type doesn't support the rev option. Their documentation dictates that the entry key contains the tag/version we'd like to use. For users that just want the latest available datree/datree:latest is ideal. If a user wants to specify a version there, they'd choose something like datree/datree:v1.42.0.

@alexfedin
Copy link
Contributor

alexfedin commented Nov 1, 2021

@MentalBrake In regards to best practices for rev key: Unfortunately it looks like the docker_image type doesn't support the rev option. Their documentation dictates that the entry key contains the tag/version we'd like to use. For users that just want the latest available datree/datree:latest is ideal. If a user wants to specify a version there, they'd choose something like datree/datree:v1.42.0.

@kevholmes I meant the rev option in the .pre-commit-config.yaml file the user will use.
for example what I used now is this, where I refer directly to this pr commit sha to pull the hook:
`
repos:

looks to me like we can use git tags for the rev value from their docs: https://pre-commit.com/#using-the-latest-version-for-a-repository
I will test it once I'll merge this pr

@alexfedin alexfedin merged commit 765f8b3 into datreeio:main Nov 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create a plugin for the pre-commit framework
4 participants