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.

Sign up for GitHub

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

Use commit.comitter instead of commit.author in validation #40

Closed
wants to merge 2 commits into from
Closed

Use commit.comitter instead of commit.author in validation #40

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e30ed3a
Use commit.comitter instead of commit.author in validation. #39
bndw Sep 29, 2017
fdb0f17
Fall back to commit.author when commit.committer not present
bndw Sep 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions lib/dco.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,17 @@ module.exports = function (commits) {
signedOff = false
defaults.failure.description = `The sign-off is missing.`
} else {
if (!validator.validate(commit.author.email)) {
let email = commit.committer ? commit.committer.email : commit.author.email
let name = commit.committer ? commit.committer.name : commit.author.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I'm seeing this, I wonder if it should attempt to check against both committer and author.

For example, I could open a pull request with:

  1. commits from me, in which case it could match either committer or author
  2. commits from someone else that already included a sign-off, in which case it would match the author
  3. commits from someone that didn't include a sign-off but I added a sign-off, in which case it would match the committer

Copy link
Author

Choose a reason for hiding this comment

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

The issue I ran into that prompted this PR was the case where I had signed a commit with one account (committer and email are the same) and later rebased and signed the commit with a different account (only committer). In this case, the author and committer will be different which was unexpected because, in my case, there was only a single commit on a PR and I could visually see it was signed by email address b (but the DCO check was looking for address a)

Copy link
Author

Choose a reason for hiding this comment

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

I agree checking both committer and author seems like the more robust solution. And, really, the above situation I got in is likely one that could be solved outside of this tool, perhaps by updating the README with more details on how this works and common troubleshooting steps.


if (!validator.validate(email)) {
signedOff = false
defaults.failure.description = `${commit.author.email} is not a valid email address.`
defaults.failure.description = `${email} is not a valid email address.`
}
match = regex.exec(commit.message)
if (commit.author.name !== match[1] || commit.author.email !== match[2]) {
if (name !== match[1] || email !== match[2]) {
signedOff = false
defaults.failure.description = `Expected "${commit.author.name} <${commit.author.email}>", but got "${match[1]} <${match[2]}>" `
defaults.failure.description = `Expected "${name} <${email}>", but got "${match[1]} <${match[2]}>" `
}
}
}
Expand Down