search

eclipse / omr

Eclipse OMR™ Cross platform components for building reliable, high performance language runtimes
http://www.eclipse.org/omr
Other
940 stars 394 forks source link

Run commit validation script with ci.eclipse.org/omr #1713

Open xliang6 opened 6 years ago

xliang6 commented 6 years ago

A while ago, I checked in a commit validation script (written in ruby) in OMR to check commit messages against the OMR contributing guidelines. However, this script needs to be installed by a developer into a local repository to allow automatic commit validation in that repository. The steps on how to do this have been outlined in the contributing guidelines, however, this is very hard to enforce.

With the new ci.eclipse.org/omr framework, should we add the ability to allow a committer/contributor to request the script to be run on the server as a part of the code review process? The script will need to be modified as our guidelines change.

xliang6 commented 6 years ago

@charliegracie @0xdaryl @mstoodle @mgaudet I think that if and when we get more contributors to the OMR and OpenJ9 projects, being able to run a commit validation script as a part of a code review would make committers' job a lot easier. What do you think? Should I look into enabling this? Thanks.

mstoodle commented 6 years ago

if it's reliable, i don't see why we wouldn't add it as a mandatory checkmark. +1

charliegracie commented 6 years ago

Can you outline what the script is checking for and what you think should cause the job to fail?

xliang6 commented 6 years ago

@charliegracie Thanks for your questions.

The script (when installed in a repository) will validate commits in the local repository, so it will have to be changed to work on the server, i.e. how it gets the commit information. It is intended to check against commit guidelines section in CONTRIBUTING.md and it needs to be updated along with the guidelines. I just noticed the guidelines were modified recently, so the script will have to be changed accordingly too.

A quick summary of what it does today:

  1. If a commit title starts with skip or fixup, script will pass without further checking. This was specially useful for the local repo scenario, it is probably not needed when the script is run on the server.
  2. If a commit title is equal or more than 50 chars, script will fail. This needs to be changed to 70 chars which is the new limit in the current guidelines.
  3. If a commit title doesn't start with a capital char or end with an alphanumeric char, script will fail;
  4. If there is no blank line between commit title and body, script will fail;
  5. If commit message body has lines of more than 72 chars, script will pass with a warning;
  6. If last line of commit message doesn't contain a Signed-off-by footer, script will fail.
charliegracie commented 6 years ago

Thanks for the write up.

  1. +1
  2. This should be similar to 5 and just print a warning.
  3. +1 I feel this is a little harsh on the starting with a capital letter but that is what is written down
  4. +1
  5. +1
  6. The Eclipse IP Validation will also check that there is a signed-off-by tag but I find the output of that tool to be a little confusing so if this tool is clear about the issue that is an improvement.

If we are going to add a job to run this script on every PR I think we may want to add some more checks to make the job of the committer and contributor easier.

  1. The script should verify that the signed-of-by name email matches at least one of the Authors. Again this is verified by the Eclipse IP tool but I think we can be more clear in our output what the issue is.
  2. The script should verify that there is a copyright header in the file. The new format of the copyright header should make this relatively easy.
  3. We should check that the copyright date has been updated to the current year.
xliang6 commented 6 years ago

@charliegracie I will update the script to reflect the changes you requested. I am not familiar with ci.eclipse.org/omr framework, can it invoke any script (mine was written in ruby)? Can you point me to a working script so I can see how github parameters are obtained in this environment? How can I test my script in this environment? Thanks.

@mstoodle @0xdaryl @mgaudet Please let me know if you would anything to be changed in the script.

charliegracie commented 6 years ago

@xliang6 the CI can easily run Ruby scripts. Are there any specific gems you would need?

For the PR builds we are using the Jenkins GitHub Pull Request Plugin so you can view the plugin help to see the data that is easily made available to consumers.

My assumption would be that I will set up the job on Jenkins to invoke your script with the require parameters. I would expect your script should just take a commit SHA and then use regular GIT commands to get any data you need about that SHA.