Add support for remediation commits

[brianwarner]

This PR adds support for remediation commits and expands test coverage. In brief, a remediation commit is an additional way for projects to recognize valid, retroactive signoffs on commits which otherwise fail the Probot DCO checks.

Remediations come in two flavors. Individual remediation commits allow an author to fix one or more failing commits by pushing an additional, properly signed-off commit with specific text indicating they apply their signoff retroactively. Third-party remediations allow someone other than the author to do this.

About this PR

First and foremost the default Probot DCO behavior is unchanged, although the default remediation text and recommendations are modified (for reasons explained in the commit message). Unless a project explicitly enables remediation commits via their repo's .github/dco.yml file, it will continue to strictly enforce the DCO and only permit commits that explicitly include a proper Signed-off-by line.

The first commit (333a52f) expands and normalizes the default test cases. Running ./node_modules/jest/bin/jest.js --verbose now returns a structured view of what we're testing, organized into success and failure scenarios. This helps ensure the default behavior remains constant through the subsequent changes.

The rest of the commits incrementally add support for remediation commits. 5aecad9 modifies and consolidates the function that explains how to fix a failing PR, because it will be important later on. I also found a corner case in testing that makes me think it's better to just recommend all default fixes be rebases instead of amends. Basically if you push multiple commits and only one fails, and it's not the most recent commit, git commit --amend is going to fix the most recent commit and isn't going to fix the issue. Since both approaches required a force-push anyhow, it seemed less error prone to just recommend a rebase.

Next, 1a43157 adds full support for remediation commits. When a commit fails, the user gets specific copy-and-paste code blocks they can use to fix the problem. In addition the test cases are increased by 4x, because in addition to testing new functionality, they also repeat all of the previous tests to ensure enabling the config options does not mess with default results.

Finally 0696f05 updates the README.

Why add this feature

At the Linux Foundation we've heard feedback from some of our communities that DCO failures are trivial for some developers to remediate, but a barrier for others. In particular, some projects have expressed concern that one-time contributors are opening PRs through the GitHub web UI, are getting blocked because they don't know to add a signoff, and are simply abandoning their contributions rather than set up a local git environment, amend their patches, and push fixes. Currently the GitHub UI does not permit authors to amend commit messages, so this was conceived as a lighter weight workaround.

In addition, this enables targeted remediations. For example, if a branch contains multiple authors, a rebase will add an author's signoff to all commits, even ones they didn't author. This may not be desirable behavior. Both individual and third-party remediations allow an author to retroactively sign off on only the commits they want to fix. This could also be used when doing a bulk remediation on a project which adopted the DCO after being open sourced.

Finally, the goal was to establish standard text that could be replicated and understood by other tools. I specifically avoided an approach that relies upon GitHub tooling (such as signing off in the comment stream or on the PR itself) because this approach embeds the remediation directly in the git log, making it both portable and permanent.

Due diligence

I recognize this is a significant addition in functionality. Over the past year I've reviewed the concept with a number of open source legal folks and some of the large projects hosted at the Linux Foundation. The feedback has been generally positive, particularly given that it can be enabled on a project-by-project basis.

Next steps

These commits attempt to preserve the existing code style to generate a minimal diff, and make it easier to see what has changed. If merged, it would make sense to follow up with a linted version.

Thanks!

---

View rendered README.md

[brianwarner]

I have to admit, I'm totally stuck on this. For some reason it's no longer reading the config file, and I'm not sure what I'm doing wrong. @hiimbex or @gr2m, it's probably some completely naive mistake on my part... Do you have any suggestions why this particular line would not be reading an optional config file at .github/dco.yml?

[gr2m]

I don't know out of hand, sorry, I'd have to take a longer look, and I don't have the time right now, sorry.

Do you see any requests happening if you set LOG_LEVEL to debug? Also the probot version used in the app is a beta version, it woudl be good to upgrade that to the latest stable version, as there were several fixes since the beta

[brianwarner]

Hmm, I upgraded all packages, and now my test instance is crashing on deploy at heroku. I switched back to master, verified it deploys properly before running npm upgrade (it does), and verified that it crashes after going from 11.0.0-beta3 to 11.1.0 (it does, unfortunately). It doesn't even get to the point of debug logging. The odd thing is that it builds and runs fine locally.

I totally understand the lack of time, and unfortunately it looks like I'm finding more problems than I'm solving. Looks like I have some homework to do.

For what it's worth, here's the error log from heroku:

2021-03-16T15:46:49.443667+00:00 heroku[web.1]: State changed from crashed to starting
2021-03-16T15:46:53.281745+00:00 heroku[web.1]: Starting process with command `npm start`
2021-03-16T15:46:56.967922+00:00 app[web.1]:
2021-03-16T15:46:56.967936+00:00 app[web.1]: > dco@1.0.0 start
2021-03-16T15:46:56.967936+00:00 app[web.1]: > probot run ./index.js
2021-03-16T15:46:56.967937+00:00 app[web.1]:
2021-03-16T15:46:58.905343+00:00 app[web.1]: INFO (server): Running Probot v11.1.0 (Node.js: v15.11.0)
2021-03-16T15:46:58.911452+00:00 app[web.1]: INFO (server): Listening on http://localhost:29480
2021-03-16T15:46:58.916887+00:00 app[web.1]: /app/index.js:8
2021-03-16T15:46:58.916888+00:00 app[web.1]: app.on(
2021-03-16T15:46:58.916889+00:00 app[web.1]: ^
2021-03-16T15:46:58.916889+00:00 app[web.1]:
2021-03-16T15:46:58.916890+00:00 app[web.1]: TypeError: Cannot read property 'on' of undefined
2021-03-16T15:46:58.916897+00:00 app[web.1]: at module.exports (/app/index.js:8:7)
2021-03-16T15:46:58.916898+00:00 app[web.1]: at Server.load (/app/node_modules/probot/lib/server/server.js:49:15)
2021-03-16T15:46:58.916898+00:00 app[web.1]: at combinedApps (/app/node_modules/probot/lib/run.js:87:20)
2021-03-16T15:46:58.916899+00:00 app[web.1]: at async Server.load (/app/node_modules/probot/lib/server/server.js:49:9)
2021-03-16T15:46:58.916899+00:00 app[web.1]: at async Object.run (/app/node_modules/probot/lib/run.js:90:9)
2021-03-16T15:46:59.067203+00:00 heroku[web.1]: Process exited with status 1
2021-03-16T15:46:59.157881+00:00 heroku[web.1]: State changed from starting to crashed

[brianwarner]

Well, I figured one thing out... the config file is read only if it was present in the repo prior to where the PR was forked. So if, for example, you make some changes that result in a dco failure, then update .github/dco.yml in the primary branch to enable the new paths, and just close/reopen the original PR, it'll not detect the changes to the config file and looks like it isn't working.

@hiimbex, I was able to use the new functionality by doing the following:

1. Make a change without signoff and open a PR, check that it fails (verifying the default case)
2. Commit a .github.com/dco.yml file to the primary branch that enables individual or individual+third party remediations
3. Make a new change without signoff and open a fresh PR, and verify that it provides individual remediations as an option

Taking a step back, I think this makes logical sense. Enabling/disabling remediation commits is a policy change that likely shouldn't be applied retroactively. And if someone opened a PR prior to enabling remediation commits, they should be able to rebase on the main branch.

[gr2m]

can we close this PR then? Can we somehow improve the docs to clarify the behavior?

[brianwarner]

Hi @gr2m, I'd like to still keep this one open for consideration if possible, please. This feature would be really helpful in some of our projects.

[bnb]

@gr2m would you be open to adding CODEOWNERS for just the features being added here?

[gr2m]

do you mean to move the new features implemented here to separate files and then use CODEOWNERS to enforce reviews / communicate ownership for these files? Sounds good to me

[gr2m]

Hi everyone! We created a DCO team with admin access to this repository and invited @brianwarner and @ryjones to the team.

The pull request is stellar and with a proper DCO maintenance team in place we don't see a reason not to go ahead and merge it. I can assist with any technical questions and problems from the perspective of the Probot framework.

It might be a good idea to deploy the version of this pull request to a test environment, in case you haven't done so yet. I can help with that as well. Please let me know how you'd like to proceed.

[ryjones]

@gr2m I'm willing to start clicking "merge" but yes let's talk about how to test this :)

[gr2m]

I've looked at the PR and current state of DCO. I'd like to address a few things before merging to master and deploying

• [x] Upgrade Probot v11-beta.3 to latest v11 (we can upgrade to v12 later)
• [x] Increase test coverage. I like to have 100% test coverage, especially for Open Source projects, to accept changes with bigger confidence and speed

For simpler collaboration, I suggest I merge this PR into a local branch, and then start a PR against that branch which addresses the points above.

Once we feel confident I'd do a preview deployment so we can test the changes before deploying them to all users of the DCO app.