Skip to content

chore: add a precommit hook for signing commit#1230

Merged
dbasunag merged 3 commits intoopendatahub-io:mainfrom
dbasunag:pre-commit-hook
Mar 18, 2026
Merged

chore: add a precommit hook for signing commit#1230
dbasunag merged 3 commits intoopendatahub-io:mainfrom
dbasunag:pre-commit-hook

Conversation

@dbasunag
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag commented Mar 16, 2026

Pull Request

Summary

pre-commit would catch locally if the current patch doesn't have signed commit

Related Issues

  • Fixes:
  • JIRA:

How it has been tested

  • Locally
  • Jenkins

Additional Requirements

  • If this PR introduces a new test image, did you create a PR to mirror it in disconnected environment?
  • If this PR introduces new marker(s)/adds a new component, was relevant ticket created to update relevant Jenkins job?

Summary by CodeRabbit

  • Chores
    • Development configuration updated to add a local commit-message hook that enforces a Signed-off-by trailer and provides error guidance when missing.
    • Removed a previously disabled configuration block related to automated tooling; no other functional changes to existing hooks or workflows.

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
@dbasunag dbasunag requested a review from a team as a code owner March 16, 2026 19:05
@github-actions
Copy link
Copy Markdown

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/verified', '/hold', '/cherry-pick', '/wip', '/build-push-pr-image', '/lgtm'}

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 9dc5e155-fb9a-4dde-936e-9cb9bc1aa45b

📥 Commits

Reviewing files that changed from the base of the PR and between 6ef4837 and 5961f75.

📒 Files selected for processing (1)
  • .pre-commit-config.yaml

📝 Walkthrough

Walkthrough

Removes a disabled Renovate pre-commit hook block and adds a local check-signoff pre-commit hook in .pre-commit-config.yaml that runs at the commit-msg stage to require a Signed-off-by: trailer in commit messages via a shell grep command.

Changes

Cohort / File(s) Summary
Pre-commit Configuration
\.pre-commit-config.yaml
Deleted disabled Renovate hook block and added a new local check-signoff commit-msg hook that runs a shell grep "Signed-off-by:" check and exits non-zero with guidance if not found.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Actionable Issues

  • Pattern enforcement is weak — the hook greps for the substring Signed-off-by: and can match occurrences anywhere in the message (false positives) or be omitted while still satisfying naive matches. Recommend using a trailer-aware check (e.g., git interpret-trailers --parse/--check or a regex anchored to a trailer line) to enforce correct placement. Reference: CWE-20 (Improper Input Validation).
  • Shell-based hook message handling — running shell commands against commit text can be brittle; ensure input is not interpreted by the shell and that the hook uses safe argument handling or dedicated tooling to avoid injection or parsing mistakes. Reference: CWE-78 (OS Command Injection) — verify the hook does not concatenate untrusted input into executed commands.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a pre-commit hook to enforce signed commits, which aligns with the file modifications in .pre-commit-config.yaml.
Description check ✅ Passed The description follows the repository template structure with all required sections present, though the Summary section is minimal and testing/requirements checklists remain incomplete.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.pre-commit-config.yaml:
- Around line 72-73: The pre-commit hook currently only checks for the presence
of "Signed-off-by:" using the grep invocation shown (bash -c 'grep -q
"^Signed-off-by:" "$1" || { ... }'), which allows malformed or empty trailers;
update that check to validate the full trailer format by replacing the simple
existence check with a regex-based validation that enforces a non-empty name and
a valid-looking email in the form "Signed-off-by: Name <user@domain>" (reject if
it doesn't match), so the hook exits with the same error when the trailer fails
this stricter pattern.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 0f37e9e9-7bc5-4e69-8702-1103e733ddfb

📥 Commits

Reviewing files that changed from the base of the PR and between 5e62a40 and 6ef4837.

📒 Files selected for processing (1)
  • .pre-commit-config.yaml

Comment thread .pre-commit-config.yaml Outdated
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Copy Markdown
Contributor

@kami619 kami619 left a comment

Choose a reason for hiding this comment

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

lgtm

@dbasunag dbasunag merged commit a9f9177 into opendatahub-io:main Mar 18, 2026
10 checks passed
@dbasunag dbasunag deleted the pre-commit-hook branch March 18, 2026 17:56
@github-actions
Copy link
Copy Markdown

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

dbasunag added a commit to dbasunag/opendatahub-tests that referenced this pull request Mar 18, 2026
* chore: add a precommit hook for signing commit

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

* fix: Update based on review comment

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

---------

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
ssaleem-rh pushed a commit to ssaleem-rh/opendatahub-tests that referenced this pull request Mar 23, 2026
* chore: add a precommit hook for signing commit

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

* fix: Update based on review comment

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

---------

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
Signed-off-by: Shehan Saleem <ssaleem@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants