search

rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
381 stars 20 forks source link

Add installation via core.hooksPath and setup new templatedir when running in non-interactive mode #19

Closed AlexGoris-Cashforce closed 5 years ago

AlexGoris-Cashforce commented 5 years ago

We noticed this project (thanks!) and wanted to use it within our organization, however to make it better suited to use in our teams, we made 2 changes:

I've updated the README.md for the 2nd point, to try to instruct users which mode (templateDir vs hooksPath) to use in which situation. Let me know if you approve or if you'd like to see something changed.

Thanks again for this awesome project!

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 228

Changes Missing Coverage Covered Lines Changed/Added Lines %
uninstall.sh 7 11 63.64%
install.sh 11 22 50.0%
<!-- Total: 18 33 54.55% -->
Totals Coverage Status
Change from base Build 205: -0.6%
Covered Lines: 1555
Relevant Lines: 1720
💛 - Coveralls
AlexGoris-Cashforce commented 5 years ago

I looked at the failed travis build, however it seems that most/all failed steps (I just spot-checked a couple) are related to docker errors (e.g. 'no such image: debian:base') Is this a known issue? I find it hard to believe that our changes caused these build failures

rycus86 commented 5 years ago

Ok, I have re-run the build now, must have been a Docker Hub flake. It flagged up some shellcheck issues + formatting: https://travis-ci.org/rycus86/githooks/jobs/555818064

Seems like you used bash in some places which would break some of the tests, and there's also an unused variable that you wanted to use maybe but then didn't?

rycus86 commented 5 years ago

Thanks for this PR!

Could you potentially pull the two changes apart and do them in one PR each? I'm totally on-board with the non-interactive change, though I think we should add an extra parameter like --allow-new-template-dir or something similar, so you'd consent to having that set up with this additional parameter rather than saying Y interactively.

For the core.hooksPath, is that a real concern or just trying to optimize the initial scanning away? This project is basically trying to solve the shared hooks problem, which you can also do with core.hooksPath but it's not necessary, since the hooks from the templates will be copied into each new Git repositories (either git init or git clone), the only longer lookup is to find the repositories that are already checked out onto the disk. It doesn't have to scan the whole disk, you can set a subfolder to start the search from. While this step can be avoided with core.hooksPath, I'm not sure if it's worth the extra complexity?

AlexGoris-Cashforce commented 5 years ago

I've pulled the non-interactive change into a separate commit.

Regarding the hooksPath changes, I tried using the script as-was, mind you we have users with repositories in multiple location over multiple drives. Also we're using windows which means we have to run the script using git-bash, which perhaps introduces some performance inssues? But on my workstation I had left the script to scan for 30+ minutes and it was still scanning for repositories. We want to provide this script to our users as a way for them to quickly setup githooks so they will start using the hooks included in our repositories, asking them to run a script which takes 30+ minutes to complete is just not realistic.

That's to provide some background on why I made the change. I also tried documenting this in the README to make new users aware of the 2 options and the pro's con's of each one.

rycus86 commented 5 years ago

I'm a bit worried about how well will this work with other parts that might assume template directories, but I'm just going to set up a test runner in Travis to run the tests with this new flag on, then we'll see (hopefully).

OK, if you rebase this branch on master or merge master into this branch, then there's a new Travis job to run the tests with the new flag: https://github.com/rycus86/githooks/commit/554495b2032384ab2652de23ca3bb1ca20e69a3a#diff-9028ab884f9d93cb10c280fe639486e7

AlexGoris-Cashforce commented 5 years ago

ugh builds are failing again because no such image: githooks:alpine and similar errors :(

Anyway, I changed the parameter to --use-core-hookspath and merged your latest master into ours.

rycus86 commented 5 years ago

That's just the last message, the actual failures are above, within the failing test cases.

> Executing step-020
  :: Run an install, and let it set up a new template directory (non-tilde)
! /var/lib/tests/step-020.sh has failed with code 1 (! Githooks were not installed into a new repo), output:
...
Initialized empty Git repository in /tmp/test20/.git/
grep: /tmp/test20/.git/hooks/pre-commit: No such file or directory
! Githooks were not installed into a new repo

Looks like tests 007 and 020 are failing.

Also, can you please change this too:

Please also rename it in the test-corehookspath.sh script for the test.

rycus86 commented 5 years ago

Based on the tests, it looks like this change does break something. Most test cases have a few steps failing, and the one that enables this everywhere has 22 failures. :( Are you OK investigating what's wrong with it or would you need any help?

AlexGoris-Cashforce commented 5 years ago

Hey @rycus86 , sorry for the late response, I was busy with a few other things. I'd like to address the errors, but in the first place I have a really hard time finding them. I always notice the error in the end saying the docker image doesn't exist.

I was able to identify a failing test: https://travis-ci.org/rycus86/githooks/jobs/560006330#L524 but that test checks if the git hooks are installed in newly created repo's, which is exactly what this PR is trying to avoid. So how can I say that test step-004.sh should be skipped for the corehookspath test?

rycus86 commented 5 years ago

No problem, thanks for picking it up again! I was also unavailable recently.

Oh, I think you're right, though haven't had time to look into the tests yet. Looks like tests 007 and 020 are failing, and as you said, they probably test something directly that wouldn't make sense with your change anymore. If you have time and could look into adapting those two tests, that would be awesome, otherwise I'll try to have a look at them today or later this week.

rycus86 commented 5 years ago

I made the changes I asked for here and fixed up the tests: https://github.com/rycus86/githooks/compare/cashforce-master?expand=1

@AlexGoris-Cashforce if you're OK with it, I'll just merge it tomorrow.

AlexGoris-Cashforce commented 5 years ago

@rycus86 Thanks for merging! Sorry for not addressing the open issues, I had a 2-week holiday and had to finish some other stuff before that. Is there still something you'd like me to look into, or are we good as is now?

rycus86 commented 5 years ago

No worries! Thanks for coming back here! I think it's all good but haven't tested it myself apart from CI, so if you get a chance to do so, it would be great if you could report back how well it works -- or not. :) Especially on Windows.

Thanks a lot!

AlexGoris-Cashforce commented 5 years ago

We are using the master branch of our repo (cashforce/githooks) on windows computers, for several weeks without issues so far. I'll update our internal documentation for new people to install from your repo starting now.