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: add commitlint command #190

Closed
wants to merge 4 commits into from
Closed

feat: add commitlint command #190

wants to merge 4 commits into from

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 12, 2018

This will use commitlint package with their config-conventional set.

After you run npm install, it sets up as a commit-msg hook which means it will run before the commit is really made to check the message. It works both on aegir repo itself and on repos that depend on aegir.

Closes #59.

(P.S.: Damn you, Windows line endings)

@ghost ghost assigned hacdias Jan 12, 2018
@ghost ghost added the status/in-progress In progress label Jan 12, 2018
@daviddias
Copy link
Member

@hacdias have you tested this with aegir-test-repo?

@hacdias
Copy link
Member Author

hacdias commented Jan 12, 2018

No, but I will. That's not the problem though. I'm having an issue with commitlint itself but that's probably my fault.

@codecov
Copy link

codecov bot commented Jan 12, 2018

Codecov Report

Merging #190 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #190   +/-   ##
=======================================
  Coverage   78.47%   78.47%           
=======================================
  Files           6        6           
  Lines         144      144           
=======================================
  Hits          113      113           
  Misses         31       31

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a67b18a...92768ee. Read the comment docs.

@hacdias
Copy link
Member Author

hacdias commented Jan 15, 2018

This seems to be working well 😄 Gonna just make it prettier

@hacdias
Copy link
Member Author

hacdias commented Jan 15, 2018

I'd like you all to take a look and merge if possible 😄

@hacdias hacdias force-pushed the feat/commitlint branch 2 times, most recently from 04fcb53 to b3dbcd1 Compare January 15, 2018 09:46
@vmx
Copy link
Member

vmx commented Jan 16, 2018

I find that the original pull request message would make a good commit message. I prefer having enough information within the Git history to see why things were done.

@hacdias
Copy link
Member Author

hacdias commented Jan 20, 2018

@vmx would you prefer not to implement this then and leave the commit message with the PR's title?

@vmx
Copy link
Member

vmx commented Jan 20, 2018

@hacdias I don't know if I understand your comment correctly :) I'm not opposed this feature, I just like having the commit message having enough information so that I can look through the git log and know why a commit was done.

@hacdias
Copy link
Member Author

hacdias commented Jan 20, 2018

@vmx oh, well, okay. It makes sense. I asked you that because I didn't understand 100% of what you said before. Sorry 😆

@diasdavid is there anything missing on this PR?

@daviddias
Copy link
Member

@hacdias have you successfully tested this with https://github.com/ipfs/aegir-test-repo? Where can I find the PR that shows that indeed it captures the linting fail?

Also, @victorbjelkholm is now the primary captain for this package given that it falls under the responsibilities of the QA, Testing and Dev Team Enablement WG, let's have him review the PR too :)

@hacdias
Copy link
Member Author

hacdias commented Jan 21, 2018

@diasdavid it fails when you run git commit -m $message and $message doesn't comply with the rules so I'm not sure how a PR on aegir-test-repo could demonstrate this.

@daviddias
Copy link
Member

@hacdias got it, aegir is installing a hook and not necessarily linting the commit messages. What happens when users use the -n option (i.e git commit -m $message -n)? Shouldn't aegir have a step that can be run on CI to validate all the commit messages and fail if they are not respected?

Also, where is the documentation for the commit messages guidelines? We need to have docs to inform users on what the rules are.

@hacdias
Copy link
Member Author

hacdias commented Jan 22, 2018

@diasdavid

The rules are here. I think I can find better examples though.

Shouldn't aegir have a step that can be run on CI to validate all the commit messages and fail if they are not respected?

Yes, they can bypass it with -n. And I don't think that doing this on a CI would be great. If, for some reason, something got on master with the wrong commit message, wouldn't it fail until the end of the times? Or would it just check the latest commit?

@victorb
Copy link
Member

victorb commented Jan 22, 2018

This is tricky. Not a fan of having it preventing commiting, as many time I write commit messages for myself first, then before pushing I do a rebase, fixing the commit messages. This would break that flow.

On the other hand, rejecting a push to my own fork because of the commit message feels wrong as well, I might just want to push something temporary.

Feels like this is better to be validated in the PR themselves via Jenkins, not locally. So we would just have a new command, lint-commits which would make sure all the commit messages are correct for that PR.

Sounds reasonable?

Edit:

If, for some reason, something got on master with the wrong commit message, wouldn't it fail until the end of the times? Or would it just check the latest commit?

It would check in the PRs to master if the commit messages in that PR is following the convention. Since everything to master goes through PR (ideally, but currently not enforced everywhere), it should protect master branch.

@vmx
Copy link
Member

vmx commented Jan 22, 2018

I agree with @victorbjelkholm. Though would it be possible to print a warning? That would be useful for newcomers like me, but won't prevent quick hacks.

@daviddias daviddias removed their request for review January 23, 2018 02:15
@daviddias
Copy link
Member

btw @hacdias did you get to try out http://commitizen.github.io/cz-cli/ ?

@hacdias
Copy link
Member Author

hacdias commented Jan 23, 2018

@victorbjelkholm @vmx yeah, it makes sense. I can change it to a command who does the linting and then we can run it on a CI. 😄

@diasdavid no, but I can check that out. Although, I'm not sure if that would be a great thing to implement here, since we would have to run something like 'yarn commit' instead of our usual 'git commit'.

@hacdias
Copy link
Member Author

hacdias commented Feb 18, 2018

So, should I change this to verify the last commit only on PRs?

@dignifiedquire
Copy link
Member

I suggest doing what we do on karma, lint on CI.

@hacdias
Copy link
Member Author

hacdias commented Mar 14, 2018

Hmm... then I need to force master to get downloaded. Thanks @vmx

@vmx
Copy link
Member

vmx commented Mar 14, 2018

@hacdias Or you drop the --depth 50 and do a remotes/origin/master...HEAD.

@hacdias
Copy link
Member Author

hacdias commented Mar 14, 2018

@vmx that way I would need to change the way every CI checks out the repo, right?

@vmx
Copy link
Member

vmx commented Mar 14, 2018

@hacdias: Good point I haven't thought about that. Just do what whatever makes most sense :)

@hacdias hacdias force-pushed the feat/commitlint branch 3 times, most recently from 68a9f8c to 7c98329 Compare March 14, 2018 14:31
@hacdias
Copy link
Member Author

hacdias commented Mar 14, 2018

@vmx perhaps you could help me: can you find a reason why's only one CI failing with yarn commitlint command not found? They were supposed to have all the same content.

@vmx
Copy link
Member

vmx commented Mar 15, 2018

@hacdias I have no clue,that really doesn't make any sense. You could perhpas add some debugging to find the issue. I would print the current commit it is on (git rev-parse HEAD) and print the contents of package.json (cat package.json).

@victorb
Copy link
Member

victorb commented Mar 27, 2018

Could you update this with the changes from master so we can get Jenkins to run tests for all platforms on this PR?

@hacdias
Copy link
Member Author

hacdias commented Mar 27, 2018

Done! CIs running.

@victorb
Copy link
Member

victorb commented May 6, 2018

@hacdias seems this is still failing:

{ Error: Command failed: git config remote.origin.fetch +refs/heads/*:refs/remotes/origin/*
warning: remote.origin.fetch has multiple values
error: cannot overwrite multiple values with a single value
       Use a regexp, --add or --replace-all to change remote.origin.fetch.

    at ChildProcess.exithandler (child_process.js:272:12)
    at ChildProcess.emit (events.js:159:13)
    at maybeClose (internal/child_process.js:943:16)
    at Socket.stream.socket.on (internal/child_process.js:363:11)
    at Socket.emit (events.js:159:13)
    at Pipe._handle.close [as _onclose] (net.js:558:12)
  killed: false,
  code: 5,
  signal: null,
  cmd: 'git config remote.origin.fetch +refs/heads/*:refs/remotes/origin/*' }

@hacdias
Copy link
Member Author

hacdias commented May 7, 2018

@victorbjelkholm I don't know what's happening here and why it does work on some CIs and not on others. It was easier to only check the last commit.

What do you think? Do you have an idea of where the error might be?

@victorb
Copy link
Member

victorb commented May 7, 2018

Seems to be a few items missing here

  • Should use yargs command structure so we can have it show in the help output
  • Should take two arguments, first from and second to, then should go through all the commit in-between (inclusive) and validate them
  • Errors need to show 1) the commit(ish) of the invalid commit and 2) the rule it broke, currently just spits out a json object without referring to which commit it it
  • No need to do git fetch from aegir, we'll handle that on the CI side instead
  • load({ extends: ['@commitlint/config-conventional'] } doesn't seem to work when outside of aegir (for example, linking aegir to js-ipfs and running ./node_modules/.bin/aegir commitlint results in error)
  • I think all the hooks/bin are superfluous. Since aegir and commitlint seems slow (takes ~10 seconds for linting 5 commits, something wrong here?), let's skip the pre-commit/push hooks for now

@hacdias
Copy link
Member Author

hacdias commented May 8, 2018

I changed the code to just use @commitlint/cli instead. It's more straightforward and I believe there will be less issues.

@hacdias
Copy link
Member Author

hacdias commented May 8, 2018

It keeps getting the same error on Travis: https://travis-ci.org/ipfs/aegir/builds/376238916#L3785.

Although Jenkins doesn't seem to throw any error, it also doesn't seem to be running the command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants