Remove checkcommits

[jodh-intel]

The checkcommits tool has served us well over the past five years to ensure that all commits comply with our documented patch format.

However, with the advent of the GitHub Actions, checkcommits has become redundant:

• https://github.com/kata-containers/tests/blob/main/.github/workflows/PR-wip-checks.yaml
• https://github.com/kata-containers/kata-containers/blob/main/.github/workflows/PR-wip-checks.yaml

Plan

• Review the functionality between the GHA and checkcommits to ensure the GHA does everything checkcommits does.
• Assuming the answer is yes:
  • Delete checkcommits.
  • Remove the checkcommits code from the static checker.
  • Update the contribution guide to mention the GHA's rather than checkcommits.

[jodh-intel]

/cc @gkurz.

[bookinabox]

Here are my initial thoughts:

It looks like functionality of checkcommits can be replaced by the GitHub action found on the main repo: https://github.com/kata-containers/kata-containers/blob/main/.github/workflows/commit-message-check.yaml

https://docs.google.com/spreadsheets/d/1ztv6TUN7BZcmhmGxyb5v6_RBHOqp0MPuoZxVG9lYpeo/edit?usp=sharing

I listed out all the features specified by the checkcommits interface on the left-most column, whether they are specified in the community guidelines next and whether I think they are currently being correctly covered by the body-regex checker.

I will do further testing to ensure everything is caught, but I suspect that since nothing has been updated to that action since March, there are at least no obvious inconsistencies with checkcommits.

[bookinabox]

@jodh-intel I'm an intern that was tasked to look at this. There was some confusion between me and the people that assigned me to look at this. To confirm, I need to confirm that this check-commit-message action made in the main repository works as intended, copy it to here, making the necessary changes, and update the documentation accordingly, correct?

[bookinabox]

Assuming that we are just testing the implementation in kata-containers/kata-containers/.github/workflows/commit-message-check.yaml, here are my thoughts after testing it:

1. The original checkcommits defaults the max body line length to 150. The commit-message-check sets it to 149. The kata community guidelines requires it to be 72, so we may want to update it.

2. There is no check for a subject's existence (though I am unsure if this is needed if there is check for the body existing). There is a subject line length check which checks for the maximum length of the subject line, but something without a subject line (e.g. empty commit message) satisfies that specific check (though it obviously fails other checks like the subsystem one so I do not think it actually matters).

3. There is a check in checkcommits not in its README that skips the body-length check that if the line is a single word (the example given in the comments being a long url). This exception is not in commit-message-check.

While I'm fairly certain 1. should be fixed, I feel like 2 is a redundant check anyways. 3 makes sense, but is not specified in the checkcommits interface or the community documentation, so it may make sense not to include it especially since there are other specified ways to bypass the body length checker (indentation/starting the line with a non-alphabetic character).

[jodh-intel]

Hi @bookinabox - Welcome to the project! Basically, this issue is to remove the existing checkcommits entirely: I've updated the title to make that clear.

Since we've been using @Tim-Zhang's https://github.com/tim-actions code in our commit-message-check for ~2 years now, it's definitely overdue for us to remove the duplication by also calling checkcommits.

Responding to your comments:

1. The original checkcommits defaults the max body line length to 150. The commit-message-check sets it to 149. The kata community guidelines requires it to be 72, so we may want to update it.

That's actually an interesting issue. 72 is probably too short as folk often paste logs into commits. However, checkcommits handles that rather neatly by allowing any length of body line, as long as it's space indented. I'm not sure what Tim's checker does but I'd consider retaining 150 and updating the docs to match the implementation (or going with 72 and ensuring Tim's code also allows the indent trick).

2. There is no check for a subject's existence

That sounds bad as we definitely need the subject and subsystem to be specified.

3. There is a check in checkcommits not in its README that skips the body-length check that if the line is a single word

Yep - that could also be useful to add to Tim's checker. That said, if we've survived for ~2 years without Tim's checker hitting the issue, maybe it's not a major problem. Ideally, Tim's checker would provide the feature though.

Tasks

This suggests we actually have three tasks. The first one is the most important but the other two are "nice to have's" if you or Tim are keen to tal at them?

• Remove checkcommits.
• Add additional checks to Tim's checker.
• Add additional unit tests to Tim's checker.

[bookinabox]

On point 1, I will continue 72 given that there does exist ways that were listed to bypass the length checker (I have checked indentation and non-alphabetic characters)

I may not have been clear on the second point. As far as I understand, the only way the subject and body can be distinguished is just by the fact that the subject is first and there is a newline between them. The subsystem check does exist and should catch anything without a subject line. I only brought it up because the checkcommits implementation does do an explicit check of the subject line existing and being non-whitespace. So it should be redundant to add another check. I just mentioned it in case there were edge-cases I was not seeing.

Thanks for the feedback and response. I know this is kind of a minor issue so I appreciate you looking at it!

Edit: Additionally, how would you recommend unit testing Github actions? I have looked into using nekto/act to run actions locally, but ran into problems. Basically, pull requests as a concept only exist on git forges rather than a local version of git and the way commits are selected to be tested is by finding all commits associated with a pull request number on github.