ci: add DCO sign-off check workflow#941
Conversation
📝 WalkthroughWalkthroughAdded a new GitHub Actions workflow that runs on pull request events and validates that every non-merge commit between the PR base and head contains a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/dco-check.yaml (1)
4-9: Add repo-level enforcement to complete the objective.This workflow alone does not prevent merges unless branch protection requires this status check (or the DCO GitHub App check) on
main.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dco-check.yaml around lines 4 - 9, The workflow "dco-check" only defines the pull_request trigger but doesn't enforce merges; update repo-level protection so the "dco-check" status check is required on branch "main" (or enable the DCO GitHub App) to block merges without DCO signoff—go to branch protection settings and add the "dco-check" workflow as a required status check for main (or install/configure the DCO app) so the pull_request runs become blocking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/dco-check.yaml:
- Around line 35-41: The current extraction uses grep -m1 to set signoff_email,
which only checks the first Signed-off-by trailer; instead collect all
Signed-off-by emails from the commit body (replace the grep -m1 usage that
assigns signoff_email) and test whether any of those emails equals author_email,
then mark the commit bad if none match; update the branch of logic that
currently compares "$signoff_email" to "$author_email" to check membership in
the list of extracted sign-off emails, keeping the existing bad+="..." and
failed=1 behavior when no match is found.
- Around line 22-33: The DCO check iterates commits using the range
"$BASE_SHA"..HEAD which can include the synthetic merge commit; fix by
explicitly targeting the PR head SHA instead of HEAD: add an env var from
github.event.pull_request.head.sha (e.g. PR_HEAD_SHA) and replace the git
rev-list range "$BASE_SHA"..HEAD with "$BASE_SHA".."$PR_HEAD_SHA" (use the
PR_HEAD_SHA env) so only real PR commits are checked; update the run block that
contains the for sha in $(git rev-list "$BASE_SHA"..HEAD) loop to use the new
PR_HEAD_SHA variable.
---
Nitpick comments:
In @.github/workflows/dco-check.yaml:
- Around line 4-9: The workflow "dco-check" only defines the pull_request
trigger but doesn't enforce merges; update repo-level protection so the
"dco-check" status check is required on branch "main" (or enable the DCO GitHub
App) to block merges without DCO signoff—go to branch protection settings and
add the "dco-check" workflow as a required status check for main (or
install/configure the DCO app) so the pull_request runs become blocking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 976577b8-89bb-4858-b945-775227544486
📒 Files selected for processing (1)
.github/workflows/dco-check.yaml
Signed-off-by: Brandon Pelfrey <bpelfrey@nvidia.com>
7790529 to
daaf3b1
Compare
Consider: skip per-commit DCO checks — enforce sign-off on the squash commit insteadThis repo is squash-merge only. That means every PR becomes a single commit on What mattersThe How to do itGitHub has a repo-level setting that controls what goes into the squash commit message body. Currently this repo uses gh api repos/nvidia/nemoclaw -X PATCH -f squash_merge_commit_message=PR_BODYWith that in place:
name: DCO Sign-off
on:
pull_request:
types: [opened, edited, synchronize]
jobs:
dco:
runs-on: ubuntu-latest
steps:
- name: Check PR body for Signed-off-by
env:
PR_BODY: ${{ github.event.pull_request.body }}
run: |
if ! echo "$PR_BODY" | grep -qP '^Signed-off-by: .+ <.+>'; then
echo "::error::PR description must contain a DCO sign-off line:"
echo "::error:: Signed-off-by: Your Name <your@email.com>"
exit 1
fi
Why this is better
The per-commit approach has a gap: even if every branch commit is signed off, the resulting squash commit on TL;DRSwitch |
Summary
Add CI DCO check which is manually verified by maintainers today.
Related Issue
Fixed #889
Changes
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Summary by CodeRabbit