boxuk / commit-message-checker Goto Github PK

Although we purposefully do not want much configuration (as the goal is to standardise commit message format across our projects), I can see some areas that would be useful:

Allowed project short-codes:

Currently we just check that there's some text where it should be. In reality for a specific project you'll want to lock it down to only allowing commit messages with the specified project code. e.g. where a ticket is actually TESTPROJ-123 the commit may accidentally contain TESTPROJECT-123 which would be considered valid.

Example config:

# .commitmessagechecker.yml
project_shortcode_whitelist:
    - TESTPROJ

Allowed ticketing systems

Most projects will just use JIRA, some will also use Zendesk. I'm not sure if we use any others, but would be a good idea to:

• By default allow Z# and J#
• Allow this to be overridden per project using config

Example config:

allowed_ticketing_systems:
    - J
    - Z

============

Anything else? We could allow commit types to be configured, but I think there's more value in enforcing consistency on that front.

Not sure how practical this will be, but seems like it would be useful to check the spelling in commit messages.

On the surface it seems straightforward enough, after all there are existing NPM modules available to handle spell-checking. However, the perceived difficulty will come with words that we want to accept, but aren't necessarily correct English (e.g. project names, technical terms, etc.).

I'm not sure there's an easy way of handling that, other than maintaining a whitelist of words (in addition to using standard dictionary). That dictionary may need to change dependent on project, which adds in a level of configurability that just doesn't exist right now.

Open to ideas / suggestions.

How should this tool work with non-Node projects? Particularly in a CI environment where Node may not be available "out of the box".

#8 is a potential solution to this, but it may not be the only one.

Open to ideas and suggestions for how to integrate this into PHP and C# projects in the easiest and least painful way possible.

Testing commit messages as part of CI is great, but is pretty frustrating for a build to get that far and then fail because of something so minor. It's very useful as a final defence to stop bad commits getting through, but it'd be better if there was a client-side hook available in addition to this.

Should create a commit-msg hook (and document its usage).

Modify reporter to output failures in a format closer to the way eslint does. In general, a more concise, clearer format. Colour could be useful, too.

We should enforce that the line length of the first line of the commit message (summary) should be <= 50 characters, and possibly that all subsequent line (description) should wrap at 72 characters.

More details on the 50/72 rule here: https://medium.com/@preslavrachev/what-s-with-the-50-72-rule-8a906f61f09c#.ns1unae7w

Currently we only check the HEAD commit, whereas ideally we'd check all commits that from the pull request.

Currently this works fine as a Node module, but not all projects will use, or have access to Node.

A future enhancement would be to write a web hook that integrates with GitHubs API. i.e. similar (but to a much, much smaller scale) to Travis/Appveyor. i.e. this would be a separate, mini-CI service.

It doesn't look that hard to do, but is very much a "nice to have" thing for the future.

Add an Appveyor CI tool, similar to the Travis one.

Should check all commits in a pull request, and the latest commit for other types of build.

For a pull request build (e.g. when merging develop into master) we use the environment variable TRAVIS_COMMIT_RANGE to work out what commits we need to check.

This seems to work correctly in most cases, however it's been observed that in some cases that SHA range (or perhaps and more likely the way we're interpreting it) will include commits that aren't part of the pull request.

Difficult to replicate and I'm not aware of any specific replication steps, but it's been seen often enough to be an issue.

We've found that when this tool is introduced to an existing project, there can be invalid commits that already exist on long running, shared branches.

For example if develop already contains many commits not yet in master, and then the commit message checker is added, when a pull request is created to merge develop into master, it can fail if any of the commits are invalid.

That may sound correct, but to fix the issue you'd have to rewrite history of the branch, which is an extremely undesirable thing to do on a shared branch. Far better to just let the invalid commits through and know that the standard will be enforced on all future commits.

This boils down to being able to ignore specific long running, shared branches. For most projects this would be master and develop, but would have to be configurable on a per-project basis (long running feature branches, etc.).

Whilst I don't like the idea of configurability for this tool (it should be consistent across projects), the goal is to enforce commit message standards and not developer workflow. With that in mind I think it would be appropriate to add this kind of feature.

I expect that this feature would require having .commit-message-checker.yml in the project root, with contents like:

ignore_pull_requests_from_branches:
    - master
    - develop
    - some-shared-branch

Note the name "ignore_pull_requests_from_branches" is quite specific. I didn't want to use ignore_branches or similar as that typically means the target branch (in services like Travis/Appveyor) and so could lead to confusion.

The vast majority of commit rules we define should cause the build to fail if they are not met. This should be a strict tool, by design, in order to enforce a consistent standard.

However, there could be cases where it would be beneficial to warn that a commit message doesn't meet a certain rule, but allow it anyway. An example may be commit length (lines), spelling (useful as a warning until we can get it to a point where there are no false positives), possibly others.

This would raise the question of how we report warnings. If it's just output in CI but the build passes, it will be effectively invisible.

Probably one for the longer term. To address when we implement a rule that we want to result in a warning.

@klj613-boxuk you requested this. Can you give some more information on:

• What makes this currently unsuitable for handling patch commits?
• An example of a commit message that fails, that should be valid
• Any other edge cases you can think of

I'll try to implement this ASAP once I have the info.