Add Signed-off-by and Assisted-By rules - #141
Conversation
Sorry, something went wrong.
|
Adding agenda for visibility, and to make sure we have consensus |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
@nodejs/tsc ... please take a look For background (in case you're unfamiliar)... When Linux Kernel first introduced the DCO, they established the convention of using Signed-off-by metadata in each commit as the way of acknowledging and attesting to the DCO for each commit. Use of Signed-off-by is the defacto standard mechanism. Node.js adopted the DCO over a decade ago but never enforced this convention. It's well past time to do so. To add the attestation, simply pass -s when committing, e.g. git commit -s. The change in this PR will enforce the presence of Signed-off-by in all commits moving foward. The format is name <email>. The rule will enforce that the <email> is a properly formatted email address. |
Sorry, something went wrong.
|
How is it going to work for automated PRs like dependencies updates? |
Sorry, something went wrong.
|
It checks and warns for those. Bots technically aren't supposed to include Signed-off-by attestation but they still do. The check accounts for that. It'll have a warning about it. |
Sorry, something went wrong.
|
@targos ... your question does make me wonder if we should just have this warn for everything at first, rather than fail outright, just in case there are cases I didn't account for correctly... But I did try to account for the known bots (like dependabot) that adds attestations.... /me second-guessing |
Sorry, something went wrong.
|
Dependabot adds the attestation, but our bot doesn't: nodejs/node#62384 |
Sorry, something went wrong.
|
Before enabling this on nodejs/node, we should probably update the commit message guidelines to clarify what it means and that it's now mandatory. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Ok the rule has been updated such that commits with the deps subsystem and backport commits do not require Signed-off-by. If a commit includes multiple subsystems other the deps, then it'll warn if no Signed-off-by is present. /cc @aduh95 |
Sorry, something went wrong.
|
I had a look at the recent commits to see if I could find some other counter example, I found nodejs/node@1baafcc, which, if it were made by a human, would be weird to sign off – but thinking more about it, we should probably simply refuse non-bot contributions there |
Sorry, something went wrong.
|
Yeah, I think those really should likely be bot updates for those. We could special case them, however... for now, let's keep it as is, requiring the Signed-off-by for these and if we can work out a better way to handle them we'll adjust. |
Sorry, something went wrong.
|
@aduh95 ... any remaining issues with this? |
Sorry, something went wrong.
This comment was marked as spam.
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-By: Opencode/Opus 4.6
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-By: Opencode/Opus 4.6
|
Given no objections, all discussions resolved, and the policy update has landed in nodejs/node main, I'm merging. |
Sorry, something went wrong.
Apply suggestion from @jasnell PR-URL: #62510 Refs: nodejs/core-validate-commit#141 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Apply suggestion from @jasnell PR-URL: #62510 Refs: nodejs/core-validate-commit#141 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Apply suggestion from @jasnell PR-URL: #62510 Refs: nodejs/core-validate-commit#141 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Apply suggestion from @jasnell PR-URL: nodejs#62510 Refs: nodejs/core-validate-commit#141 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Apply suggestion from @jasnell PR-URL: #62510 Refs: nodejs/core-validate-commit#141 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Apply suggestion from @jasnell PR-URL: #62510 Refs: nodejs/core-validate-commit#141 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Apply suggestion from @jasnell PR-URL: #62510 Refs: nodejs/core-validate-commit#141 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Add validation rules for Signed-off-by and Assisted-by footers.
BEGIN_COMMIT_OVERRIDE
feat: add Signed-off-by and Assisted-By rules
END_COMMIT_OVERRIDE