Skip to content

Conversation

jwnrt
Copy link

@jwnrt jwnrt commented Jan 23, 2025

This PR adds a script which lints commit messages for the title format and the signed-off-by line.

I'm not sure I've gotten the title format correct. Is it:

[ot] hw/opentitan: description?

Or is there an extra part for the IP block that was changed?

[ot] hw/opentitan: ot_hmac: description

If this shell script is too unreadable I don't mind it moving to Python, it's just that I don't know Python...

Fixes #1 and fixes #2

@rivos-eblot
Copy link

[ot] hw/opentitan: ot_hmac: description

We've been using the above syntax (from QEMU), that is dir1/dir2: component: description prefixed with [ot] .

@rivos-eblot
Copy link

If this shell script is too unreadable I don't mind it moving to Python, it's just that I don't know Python...

Shell seems ok for now.

I'm a bit concerned with the checkout as QEMU history is quite large (and that the public CI runners are deadly slow :-)). It is ok for now, but would it be possible to only query the log messages for all the PR commits rather than the full code base? I guess it cannot be done with actions/checkout though :-(

@jwnrt
Copy link
Author

jwnrt commented Jan 23, 2025

I think we could set an upper bound and hope that no PR goes over, say, 100 commits.

The commit hashes might be wrong in the error messages though.

@rivos-eblot
Copy link

Maybe it is possible to get the origin/destination branch (through Git Hub API) and use the git CLI to retrieve the info.

It guess that for now, it is nevertheless far better than having no lint at all, so let's merge it and we could improve it later.

@rivos-eblot
Copy link

Note: we will need some escape/bypass mechanism, such as for #116

@jwnrt
Copy link
Author

jwnrt commented Jan 23, 2025

I think committers should be able to click merge even when this job fails if they know it's okay

@jwnrt
Copy link
Author

jwnrt commented Jan 23, 2025

Alright, I changed the format to `[ot] path/name: component: description' but left the git checkout the same.

Copy link

@rivos-eblot rivos-eblot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jwnrt jwnrt force-pushed the lint-commits branch 3 times, most recently from 0c40cda to c840a05 Compare January 23, 2025 16:18
Signed-off-by: James Wainwright <james.wainwright@lowrisc.org>
@jwnrt jwnrt merged commit e68a395 into lowRISC:ot-earlgrey-9.1.0 Jan 23, 2025
7 checks passed
@jwnrt jwnrt deleted the lint-commits branch January 23, 2025 17:24
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.

Create pre-submit check for Signed-off by line Create pre-submit check for '[ot]' prefix

3 participants