-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Use pre-commit for Git hooks #3406
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
Conversation
|
@andrewhare Looks really good! Thank you so much for the PR! For the record: We've discussed during the meeting with @andrewwhare, that it might be worth creating those hook files somewhere (e.g. in .dvc/ or .dvc/tmp/) and triggering those from pre-commit config, instead of writing shell wrapper directly into them. |
|
@efiop Thanks for the feedback! I like the idea of writing the hook files to disk (possibly reusing/refactoring |
23dae14 to
7c0792f
Compare
dvc/scm/git/__init__.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we really need to run pre-commit for the user, maybe config is enough + a hint now run 'pre-commit install' to install git hooks? Just a question.
dvc/scm/git/__init__.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is needed in case user has .pre-commit-hooks.yaml, but no config? Or am I missing something?
pared
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding test asserting that hooks work.
I tried to check their behavior and upon running this script:
#!/bin/bash
rm -rf repo storage git_repo
mkdir repo git_repo
main=$(pwd)
pushd git_repo
git init --quiet --bare
popd
pushd repo
git init --quiet
dvc init -q
git remote add origin $main/git_repo
dvc remote add -d str $main/storage
dvc install --use-pre-commit-tool
echo data >> data
dvc add data -q
git add -A
git commit -am "something"
I get the following error:
An error has occurred: InvalidConfigError:
==> File .pre-commit-config.yaml
==> At Config()
==> At key: repos
==> At Repository(repo='local')
==> At key: hooks
==> At Hook(id='dvc-post-checkout')
==> At key: stages
==> At index 0
=====> Expected one of commit, commit-msg, manual, merge-commit, prepare-
commit-msg, push but got: 'checkout'
Check the log at /home/prd/.cache/pre-commit/pre-commit.log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some particular reason to use unittest? We re moving towards using pytest, I think it would be good to make it test also pytest-based.
|
@pared Did you install the latest pre-commit version? As mentioned in the header, it requires 2.2.0 EDIT: can reproduce, looking into it. |
89fd2a5 to
24967a8
Compare
|
For the record: taking over this PR. |
b226826 to
cf2fa6d
Compare
|
Currently you need to |
9891552 to
df29cd2
Compare
ac7780d to
1490809
Compare
1490809 to
63fcc66
Compare
| repo: https://github.com/andrewhare/dvc | ||
| rev: WIP-pre-commit-tool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be fixed after merge.
|
Ok, merging to adjust on master. So far works good for me, but it is tied to github repo, so need to try out on our master. Docs will come separately after. |
|
@efiop do we need adjust any docs because of this change? |
|
@shcheklein Yes, hence "Docs will come separately after." |
| formatter_class=argparse.RawDescriptionHelpFormatter, | ||
| ) | ||
| install_parser.add_argument( | ||
| "--use-pre-commit-tool", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to have --use-pre-commit or just --pre-commit. Tool is kind of redundant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skshetry Im worried about it being confused with pre-commit hooks.
Fixes #3271.
NOTE: This is blocked by a pending
2.2.0release ofpre-committo supportpost-checkouthooks.This introduces a new flag to the
dvc installcommand:Running this command will do several things:
pre-commitis installed on the system.dvc/tmp/hooksinstead of to.git/hookspre-commitconfiguration that calls the hook files in.dvc/tmp/hooks..pre-commit-config.yaml, if one is found, merge the configuration into it, otherwise dump the configuration to a new.pre-commit-config.yaml.