WIP: git hooks support and run tests before push #14
Conversation
|
@antonok-edm what do you think? |
There was a problem hiding this comment.
Sorry for the delay!
This looks like a good start, although it didn't work when I tested it as follows:
git checkout feature/verify-on-push
npm install
git checkout -b 'test-pr14'
# edited `filter_lists/default.json` to include non-JSON contents
git add filter_lists/default.json
git commit
git push -u origin test-pr14
It doesn't appear to have run npm test in the hook at all - that would have failed, and I was still able to push to test-pr14, as you can see here.
.git-hooks/pre-push
Outdated
| @@ -0,0 +1,3 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| git diff HEAD --name-only --diff-filter=ACM | grep '\(resources\|metadata\).json$' | xargs npm run prepush --quiet | |||
There was a problem hiding this comment.
What's the reason for all the extra diff/grep/xargs mechanics? If I understand correctly, you're passing the modified file paths to npm run prepush, which doesn't change how it will work - verify.js doesn't read any command line arguments.
I think just npm run prepush would be fine here?
There was a problem hiding this comment.
Ah, I see your explanation above now
executes
npm testbefore push if eithermetadata.jsonordist/resources.jsonhave changes
That list should at least include filter_lists/default.json and filter_lists/regional.json, although I think I'd prefer to just run it on any push just in case some of the file structure changes. The tests finish extremely quickly anyways 😄
There was a problem hiding this comment.
Great! Running every time makes much more sense now. Just updated with this change 😄
I have restricted to those files due to the README instructions, perhaps we could add that the tests are executed before every push now?
| "test": "node verify" | ||
| "test": "node verify", | ||
| "prepush": "npm run test", | ||
| "postinstall": "if [ -d .git-hooks ]; then for file in `ls .git-hooks`; do ln -sf ../../.git-hooks/${file} .git/hooks/${file}; done fi" |
There was a problem hiding this comment.
If we're checking that .git-hooks is a directory, we should probably also check that .git/hooks is a directory.
There was a problem hiding this comment.
Actually, we are checking for the .git-hooks directory just to find out if we should install the customized hooks. Since this directory is ignored when the library is installed, we won't be installing and executing our hooks in the consumer projects.
As for the .git/hooks I have assumed that the project is always initialized as .git repository, do you think it makes sense to change this behaviour?
|
Also, we don't need Windows support, but cross-platform is always nice if it's not too much extra complexity. |
No worries! Actually, I am sorry for the biggest delay now haha, I have been through some changes and had to take some time off. Now I'm back 😁
I understand. I suppose the only part that could cause a problem is the hooks installation. I will check how that behaves on Windows and explore our options if it's too much work we can move on to a more sophisticated approach such as Husky. Could you test again after the changes to always run, please? |
WIP: this is a draft so that we can discuss the approach 😁 I am also wondering if we should support Windows?
post-installeventnpm testbefore push if eithermetadata.jsonordist/resources.jsonhave changesREADME.mdCloses #1