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

C H E C K S A P I S U P P O R T 🀩 #79

Merged
merged 18 commits into from
Aug 22, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
74 changes: 67 additions & 7 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const getDCOStatus = require('./lib/dco.js')
const requireMembers = require('./lib/requireMembers.js')

module.exports = app => {
app.on(['pull_request.opened', 'pull_request.synchronize'], check)
app.on(['pull_request.opened', 'pull_request.synchronize', 'check_run.rerequested'], check)

async function check (context) {
const config = await context.config('dco.yml', {
Expand All @@ -19,13 +19,73 @@ module.exports = app => {
head: pr.head.sha
}))

const dcoParams = await getDCOStatus(compare.data.commits, requireMembers(requireForMembers, context))
const commits = compare.data.commits
const dcoFailed = await getDCOStatus(commits, requireMembers(requireForMembers, context), context.issue())

const params = Object.assign({
sha: pr.head.sha,
context: 'DCO'
}, dcoParams)
if (!dcoFailed.length) {
return context.github.checks.create(context.repo({
name: 'DCO',
head_branch: pr.head.ref,
head_sha: pr.head.sha,
status: 'completed',
conclusion: 'success',
completed_at: new Date(),
output: {
title: 'DCO',
summary: 'All commits are signed off!'
}
}))
} else {
let summary = []
dcoFailed.forEach(function (commit) {
summary.push(`Commit sha: [${commit.sha.substr(0, 7)}](${commit.url}), Author: ${commit.author}, Committer: ${commit.committer}; ${commit.message}`)
})
summary = summary.join('\n')
if (dcoFailed.length === 1) summary = handleOneCommit(pr, dcoFailed) + `\n\n${summary}`
else summary = handleMultipleCommits(pr, commits.length, dcoFailed) + `\n\n${summary}`

return context.github.repos.createStatus(context.repo(params))
return context.github.checks.create(context.repo({
name: 'DCO',
head_branch: pr.head.ref,
head_sha: pr.head.sha,
status: 'completed',
conclusion: 'action_required',
completed_at: new Date(),
output: {
title: 'DCO',
summary
},
actions: [{
label: 'Set DCO to pass',
description: 'would set status to passing',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to get eyes on this from a legal perspective to make sure that a this is worded properly. Something like "Accept DCO" and "I certify that my contributions fall under the Developer Certificate of Origin: https://developercertificate.org/"

@caniszczyk @mlinksva: could you recommend someone to help here?

Also, I wonder if there should be a separate "Dismiss" vs "Accept" buttons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's important to note this option is only accessible for users with write access to the repo.

We get quite a few requests for new users to not have to use git so painfully, so if the maintainers of a repo are happy with the user's comments that they "accept" the DCO, might as well give them a free status pass button.

Copy link

@mlinksva mlinksva Jul 9, 2018

Choose a reason for hiding this comment

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

Maybe I'm missing something, but this looks to me like a way for a maintainer to ignore the test, not for a contributor to agree their contributions are covered by the DCO.

Added: as @hiimbex wrote faster. 😳

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh, sorry, I totally wasn't thinking about who would be the one clicking the button.

identifier: `override`
}]
}))
}
}

// This option is only presented to users with Write Access to the repo
app.on('check_run.requested_action', setStatusPass)
async function setStatusPass (context) {
return context.github.checks.create(context.repo({
name: 'DCO',
head_branch: context.payload.check_run.check_suite.head_branch,
head_sha: context.payload.check_run.head_sha,
status: 'completed',
conclusion: 'success',
completed_at: new Date(),
output: {
title: 'DCO',
summary: 'Commit sign-off was manually approved.'
}
}))
}
}

function handleOneCommit (pr, dcoFailed) {
return `You only have one commit incorrectly signed off! To fix, head to your local branch and run: \n\`\`\`bash\ngit commit --amend --signoff\n\`\`\`\nNow your commits will have your sign off. Next run \n\`\`\`bash\ngit push --force origin ${pr.head.ref}\n\`\`\``
}

function handleMultipleCommits (pr, commitLength, dcoFailed) {
return `You only have ${dcoFailed.length} commits incorrectly signed off! To fix, head to your local branch and run: \n\`\`\`bash\ngit rebase HEAD~${commitLength} --signoff\n\`\`\`\n Now your commits will have your sign off. Next run \n\`\`\`bash\ngit push --force origin ${pr.head.ref}\n\`\`\``
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes the commits missing the DCO are always the last n commits. What if I have 5 commits, but the one in the middle properly has the DCO?

Could we just have the user rebase based on the base of the pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong, butgit push --force origin ${pr.head.ref} does exactly what you're describing. I was originally trying to do something fancy with commitLength but I think it's actually dead code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I forgot what I actually did. So commitLength actually is the number of total commits in the PR, so you always rebase and sign off all of those commits, no matter which ones were wrong via:
git rebase HEAD~${commitLength} --signoff which utilizes this rebase option. And then does:
git push --force origin ${pr.head.ref}

}
46 changes: 26 additions & 20 deletions lib/dco.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
const validator = require('email-validator')

// Returns the DCO object containing state and description
// Also returns target_url (in case of failure) in object
module.exports = async function (commits, isRequiredFor) {
// Returns a list containing failed commit error messages
// If commits aren't properly signed signed off
// Otherwise returns an empty list
module.exports = async function (commits, isRequiredFor, prInfo) {
const regex = /^Signed-off-by: (.*) <(.*)>$/im
const failure = (description) => {
return {
state: 'failure',
description: description.substring(0, 140),
target_url: 'https://github.com/probot/dco#how-it-works'
}
}
let failed = []

for (const {commit, author, parents} of commits) {
for (const {commit, author, parents, sha} of commits) {
const isMerge = parents && parents.length > 1
const signoffRequired = !author || await isRequiredFor(author.login)
if (isMerge || (!signoffRequired && commit.verification.verified)) {
Expand All @@ -22,23 +17,34 @@ module.exports = async function (commits, isRequiredFor) {
}
const match = regex.exec(commit.message)

const commitInfo = {
sha,
url: `https://github.com/${prInfo.owner}/${prInfo.repo}/pull/${prInfo.number}/commits/${sha}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using context.payload.pull_request.html_url as the base for this?

author: commit.author.name,
committer: commit.committer.name,
message: ''
}

if (match === null) {
if (signoffRequired) return failure(`The sign-off is missing.`)
if (!commit.verification.verified) return failure(`Commit by organization member is not verified.`)
if (signoffRequired) {
commitInfo['message'] = `The sign-off is missing.`
failed.push(commitInfo)
} else if (!commit.verification.verified) {
commitInfo['message'] = `Commit by organization member is not verified.`
failed.push(commitInfo)
}
} else {
if (!(validator.validate(commit.author.email || commit.committer.email))) {
return failure(`${commit.author.email} is not a valid email address.`)
commitInfo['message'] = `${commit.author.email} is not a valid email address.`
failed.push(commitInfo)
}
const authors = [commit.author.name.toLowerCase(), commit.committer.name.toLowerCase()]
const emails = [commit.author.email.toLowerCase(), commit.committer.email.toLowerCase()]
if (!(authors.includes(match[1].toLowerCase())) || !(emails.includes(match[2].toLowerCase()))) {
return failure(`Expected "${commit.author.name} <${commit.author.email}>", but got "${match[1]} <${match[2]}>".`)
commitInfo['message'] = `Expected "${commit.author.name} <${commit.author.email}>", but got "${match[1]} <${match[2]}>".`
failed.push(commitInfo)
}
}
}
return {
state: 'success',
description: 'All commits have a DCO sign-off from the author',
target_url: 'https://github.com/probot/dco#how-it-works'
}
return failed
}
Loading