Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW] Add pre-commit hook for DCO and pre-commit check #319

Merged

Conversation

VibhuJawa
Copy link
Collaborator

This pull request introduces improvements to the .pre-commit-config.yaml file, focusing on enhancing the pre-commit hook configuration. This PR closes: #290

Key updates:

  • Ensure commits are signed-off, enforcing commit message compliance.
  • Remind developers to install pre-commit hooks if missing, reducing setup errors.

Example Usage:

(nemo_curator_dev) vjawa@dgx11:~/NeMo-Curator$ git add .pre-commit-config.yaml
(nemo_curator_dev) vjawa@dgx11:~/NeMo-Curator$ git commit -m 'check if signoff warning is raised'
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check yaml...............................................................Passed
detect private key.......................................................Passed
fix end of files.........................................................Passed
Pre-commit Installation Reminder.........................................Passed
Check Signed-off-by......................................................Failed
- hook id: check-signoff
- exit code: 1

❌ Commit message must be signed off. Use git commit -s to add it automatically.

Additional example of running pre-commits

(base) vjawa@dgx11:~/NeMo-Curator$ git commit -m 'check if signoff warning is raised'
`pre-commit` not found. Did you forget to activate your virtualenv?

Signed-off-by: Vibhu Jawa <vibhujawa@gmail.com>
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Looks good on my end. Is there a similar way we can check for signing commits using -S?

@VibhuJawa
Copy link
Collaborator Author

Looks good on my end. Is there a similar way we can check for signing commits using -S?

Unsure what you mean here. This is all ready run at a per commit level is my understanding.

@ryantwolf
Copy link
Collaborator

ryantwolf commented Oct 23, 2024

This precommit checks for -s if I'm not mistaken. We need the user to sign and sign-off their commits using git commit -sS. Is there any way for the gpg signing to be detected in the precommit?

@ayushdg ayushdg added the meta General NeMo-Curator maintenance/packaging label Oct 23, 2024
@VibhuJawa
Copy link
Collaborator Author

Is there any way for the gpg signing to be detected in the precommit?

Ahh, cool, let me see how to add that too.

@VibhuJawa VibhuJawa self-assigned this Oct 23, 2024
@VibhuJawa
Copy link
Collaborator Author

@ryantwolf , I checked but could not find a satisfactory way to check for GPG. I think lets merge this PR as is, I will file an issue to track a cleaner way to sign GPG

@ryantwolf
Copy link
Collaborator

@VibhuJawa ok no worries, I was just curious. This is still good on my end, so feel free to merge whenever.

@VibhuJawa VibhuJawa merged commit c60c6e6 into NVIDIA:main Oct 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta General NeMo-Curator maintenance/packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ADD DCO signing as a pre-commit check
3 participants