Skip to content
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

[rush] Add support for git hooks #916

Merged
merged 6 commits into from Nov 7, 2018

Conversation

Projects
None yet
4 participants
@nchlswhttkr
Copy link
Contributor

commented Oct 30, 2018

Resolves #821

Feedback on this would be welcome, I'm not sure how to test this during runtime.

At the moment, I'm just copying over any files in common/git-hooks to the git config, so stricter rules could be applied if needed.

Is there any need to worry about clearing the .git/hooks directory before writing to get rid of outdated scripts? FileSystem.copyFile should overwrite automatically, but that won't get rid of old files if a replacement does not exist.

Happy hacktoberfest!

nchlswhttkr added some commits Oct 30, 2018

@nchlswhttkr nchlswhttkr changed the title Added support for git hooks [rush] Added support for git hooks Oct 30, 2018

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Oct 30, 2018

@nchlswhttkr cool, thanks for making this PR! :-)

My main feedback is that this should probably be a repo-wide configuration, rather than a command-line option. At least, in the use cases that I've observed, the repo maintainer would want to set this up for all developers (e.g. to normalize their code using Prettier), rather than it being an "opt-in" feature that happens when they install.

One way to do that would be via the rush.json config file. Although in the original issue it occurred to me that the presence of the common/git-hooks/ folder might be sufficient. #Resolved

@nchlswhttkr

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

I agree with that. I think having a common/git-hooks/ folder is sufficient a repo to opt-in, so long as the behaviour is documented.

For now, I can change to logic to run by default and log when it detects and copies over any new/updated git hooks. #Resolved

@octogonz octogonz changed the title [rush] Added support for git hooks [rush] Add support for git hooks Oct 31, 2018

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

Is there any need to worry about clearing the .git/hooks directory before writing to get rid of outdated scripts? FileSystem.copyFile should overwrite automatically, but that won't get rid of old files if a replacement does not exist.

Yes, I think this makes sense, for a couple reasons:

  • If the repo maintainer deletes an unneeded hook, but the file remains in the person's .git folder, then when that hook is invoked it may fail because the script it's pointing to no longer exists. That would cause a lot of headaches. So the default behavior should be to delete.

  • Some people may want to install their own additional Git hooks, which are no managed by Rush. In this case they need an config file or environment variable to override the default behavior. There are some design questions. If/when someone asks for that, they should have the burden of figuring that out. :-)
    #Resolved

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

One edge case would be if NO hooks are registered with Rush. In this case, it might be safer to do nothing, rather than deleting all the Git hooks. (?) #Resolved

nchlswhttkr added some commits Nov 2, 2018

Modified installation behaviour for git hooks
Hooks are now installed when the /common/git-hooks folder is non-empty.

Any hooks already installed will be deleted before new hooks are added.
@nchlswhttkr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

I've updated the PR accordingly, for now it clears any existing hooks before doing an install.

Having the /common/git-hooks folder is still what triggers the opt-in behaviour, but it will warn and skip if the folder is empty.

For now, Rush is just providing a central location to store and manage repo-wide git hooks. If we want to extend beyond this with more configuration options, that discussion can occur another time when we know the requirements better. #Resolved

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

Awesome! @cliffkoh @kenotron would you guys want to try this in the OUIFR repo?

It's probably more efficient than Husky. If I remember right, Husky executes shell scripts for every single Git event, regardless of whether there is actually a handler or not. #Resolved

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 6, 2018

Hey @nchlswhttkr, sorry it took a while to follow up. We wanted someone to test it out in a real repo before we merge the feature. I just chatted with @natalieethell who is going to give this a try in the office-ui-fabric-react project, which uses Git hooks. Then we'll finally merge it. Thanks for your patience! #Resolved

@nchlswhttkr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

No worries 👌 #Resolved

@natalieethell

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

Just finished up trying this out in the office-ui-fabric-react project, and it seems to work as expected. Everything in .git\hooks is deleted and/or replaced with the files in common\git-hooks during a rush install.

Additionally, if common\git-hooks is empty, nothing changes in .git\hooks, as expected, and a warning is displayed. #Resolved

@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2018

Awesome, thanks a bunch @natalieethell ! I'll merge this and include it with the next Rush release #Resolved

pgonzal added some commits Nov 7, 2018

Test whether common/git-hooks contains any files, since the existence…
… of the folder is not determined by Git
@octogonz
Copy link
Collaborator

left a comment

:shipit:

@octogonz octogonz merged commit 7b30b58 into microsoft:master Nov 7, 2018

3 checks passed

code-review/pullapprove Approved by pgonzal
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@octogonz

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2018

@nchlswhttkr FYI this was published as part of Rush 5.5.1

I also wrote some documentation here: https://rushjs.io/pages/maintainer/git_hooks/

Thanks again for implementing this!

@nchlswhttkr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

You're welcome, have a good one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.