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

DEV: Replace Overcommit with Lefthook #7826

Merged

Conversation

@Arkweid
Copy link
Contributor

commented Jun 28, 2019

Overcommit uses prebuilt hooks and require global installation.
To avoid this issues replace it with Lefthook.
Lefthook will be installed with npm packages. New contributors
will have fully consistent git hooks.

Goals:

  • Strict workflow. Avoid situations when newcomers pushed PR without checks with rubocop or eslint. And members should recommend to install overcommit.
  • Flexibility. You free to combine basic operations.
  • Сlarification. Overcommit provide prebuilt hooks. So its hard to understend what is going on without inspecting overcommit repository.
  • 20% Faster. Well, it not a big issue in development environment, but a like faster instruments :)
  • Twice speed up for CI lints. With Lefthook we can run lints on CI in paralel mode. You can check example here.

Example output for this commit:
PR example

Example output for CI lints:
PR example

Don't forget to remove preinstalled hooks from you repo overcommit --uninstall if you want to test it action.

So what do you think about. Is it worth?

DEV: Replace Overcommit with Lefthook
Overcommit uses prebuilt hooks and require global installation.
To avoid this issues replace it with Lefthook.
Lefthook will be installed with npm packages. New contributors
will have fully consistent git hooks.
@discoursebot

This comment has been minimized.

Copy link

commented Jun 28, 2019

You've signed the CLA, Arkweid. Thank you! This pull request is ready for review.

@ZogStriP

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

I 😍 that it also runs prettier.

@gschlager

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@Arkweid How does Lefthook handle Security? I like that overcommit uses signature checking to prevent arbitrary commands from running on my system without me knowing about it.

@Arkweid

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@gschlager thank you for this question. Lefthook uses signatures to keep hooks up to date. And does not use them for security purposes.
My opinion is that this feature does not bring any security improvements.
When you clone the project like discourse, you should to run several commands yarn install and bundle install. And you hope none of 300+ npm dependencies don't do something terrible. Same for gems. With installation process it runs Make file(for native extensions) and who know what is going inside. You again hope it's ok.

Instruments like Overcommit hide hooks implementation inside. So when developers see changes in .overcommit.yml they must sign these changes, and again they hope it's ok.

Example:

PreCommit:
  YamlSyntax:
    enabled: true

To understand what exactly going on here you should look at the code in overcommit repository. And between versions, this code can be changed. We can only hope the new code will not try to break something. So, I see only one benefit from the sign option it: "Hey, .overcommit.yml changed. Do you trust it?".

For my opinion, the only way to achieve security - install project into Docker container.


But if you think that this is still an important feature, then I can add it. Just let me know.

@ZogStriP ZogStriP merged commit 0872a11 into discourse:master Jul 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ZogStriP

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Let's try this 👍

@discoursebot

This comment has been minimized.

Copy link

commented Jul 2, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/lefthook-support/121929/3

@Arkweid Arkweid deleted the Arkweid:replace-overcommit-with-lefthook branch Jul 3, 2019

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