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

feat(check commit): add pre-commit and hook #12

Closed
wants to merge 1 commit into from

Conversation

saugereau
Copy link

Use npm to have a feature of git hook checking for conventional commits.
Pro : a ready-to-use solution
Cons : introduce package.json in the project
But with this kind of solution we can have the same requirement on commits for front and back projects ...
Maybe will it be interesting to add this kind of feature in our gradle-quality-plugin ...?

@leomillon
Copy link
Collaborator

I'm not really a fan of having to install NPM to just have commit format validation...

Especially handled by Gradle on the build task:

  • I can commit without running the Gradle build command (I know that I'm supposed to run it "every time")
  • If I just want to build without any intention of creating new commit(s), it will install NPM anyway

So this feature could be interesting if it was included inside the git repository directly (without NPM tooling) or maybe in the CI process (validating the commit history during the pull-requests).

@clemstoquart , @celcius112 , any other feedback regarding this "feature"?

@leomillon leomillon added question Further information is requested enhancement New feature or request labels Nov 27, 2019
@clemstoquart
Copy link
Collaborator

Hi guys,

I agree with @leomillon , I'm not comfortable to delegate that kind of check to a node module.

@saugereau
Copy link
Author

Wouldn't it be interesting to develop our own plugin ? :)

@leomillon
Copy link
Collaborator

I think it should probably be a git hook configuration only (nothing managed by Gradle or any other building tool)

And finally, even if it was a git configuration only, the pre-commit hook would quickly annoy me as I'm creating a lot of commit during my local development that does not always follow the "conventions" before I rework them completely for the final pull-request push so...

In my opinion, the only proper solution could be a custom scan of pull-request commits during the CI build...

@celcius112
Copy link
Collaborator

celcius112 commented Nov 27, 2019

I agree too with leo. It seems there already are solutions for enforcing git rules during the CI build. A shame that we don't have similar capabilities as in Gitlab for this.
However as seb suggested it might be interesting to find a solution with Gradle, or event create our own solution. The advantage being that a plugin might be shared more easily with other projects

@Crow-EH
Copy link

Crow-EH commented Nov 29, 2019

Don't forget that using a standard commit linter is cool because we have access to standard rules, standard tools using standard commit formats etc. without reinventing anything.

I think "Where we plug it" (local exec with git hook VS PR CI) might be the most important question.

By the way @leomillon, you can use --no-verify to skip the pre-commit hook if necessary, hence the common practice of mirroring pre-commit checks with CI checks.

@leomillon
Copy link
Collaborator

@Crow-EH I understand the goal of this approach to have a "cleaner" commit historic but, on the day to day work process:

  • I don't see how this commit message will be really helpful for "tooling integration"
  • I don't want to have to worry about adding --no-verify to bypass systematic controls, the same way I don't want to have automatic ./gradlew build running when I change 1 character in my code: I want to be able to run it when I want

The overall picture, to me, seems a bit too "strict" with too many constraints and installation costs when we can just say: the merge-request is there to ensure that everything is in order to be merged in the project history.

@leomillon leomillon closed this Feb 18, 2020
@leomillon leomillon deleted the ekino/pre-commit-hook-linter branch August 18, 2020 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants