Add support for remediation commits #147
Conversation
There was a problem hiding this comment.
Changes look good to me overall. I'm just concerned that it adds quite some complexity, I am worried about the maintenance of the app. As you use the app at the Linux Foundation, I wonder if you or someone else from your team(s) could help out maintaining DCO, and especially the advanced features added by this PR?
There was a problem hiding this comment.
So I tested current functionality and feel pretty comfortable merging considering this is an opt in feature, especially with @brianwarner offering to help maintain this feature.
I skimmed the actual logic changes but don't really care to nitpick- would love a follow up styling/refactoring PR - rereading this code hurts my brain 🙈
I did try to test the new code path, but can't quite get the scenario correct? Left an inline comment where i got stuck.
The tests look fantastic - really appreciate the thoroughness.
|
|
||
| let rebaseWarning = '' | ||
|
|
||
| if (allowRemediationCommits.individual || allowRemediationCommits.thirdparty) { |
There was a problem hiding this comment.
When testing the feature, I never made it into this if statement - here's the yaml file i used: https://github.com/hiimbex/Test/blob/master/.github/dco.yaml & the test PR i created: hiimbex/Test#4 - totally could be something wrong in how I'm testing, so let me know, but this scenario was my interpretation from the docs.
There was a problem hiding this comment.
Hmm, this is odd. I've been staring at this and must be missing something obvious... it was working earlier in development and appears to have stopped now. It's like it isn't reading the dco.yml file correctly and is always defaulting to the false values for both. Does anything jump out around line 20?
I guess this wasn't surfaced in the dco.js tests because those two variables are defined independent of what's in dco.yml for each test. I'm coming up against the limits of my knowledge of jest here, and suspect this would have been detected had there been appropriate snapshot tests that include permutations of dco.js. I'll admit I don't know how to do this aside from literally changing the default value in index.js, do you have any suggestions?
| } | ||
|
|
||
| function getSignoffs (commit) { | ||
| const regex = /^Signed-off-by: (.*) <(.*)>$/img | ||
| function getSignoffs (comparisonCommit, comparisonSha, allCommits, allowRemediationCommits) { |
There was a problem hiding this comment.
I find the naming comparison a bit confusing - this is referring to the single commit that's currently being processed correct? esp when we get into this area it gets pretty hard to parse.
There was a problem hiding this comment.
Yes, that's correct and it's a good point. It is confusing. currentCommit and currentSha might be better? As in, this is the current commit we're processing in an attempt to find valid remediation text in all other commits.
|
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 |
|
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 |
|
Hmm, I upgraded all packages, and now my test instance is crashing on deploy at heroku. I switched back to 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: |
|
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 @hiimbex, I was able to use the new functionality by doing the following:
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. |
|
can we close this PR then? Can we somehow improve the docs to clarify the behavior? |
|
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. |
|
@gr2m would you be open to adding CODEOWNERS for just the features being added here? |
|
do you mean to move the new features implemented here to separate files and then use |
|
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. |
|
@gr2m I'm willing to start clicking "merge" but yes let's talk about how to test this :) |
|
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
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. |
This commit makes minimial changes to Probot DCO itself, and does not add any features aside from adding the author's email to the commit object that gets returned on a failed check, and allowing properly signed-off commits with trailing whitespace. The goals of this commit are: * Expand text coverage (by approx 2x) * Standardize how tests are named * Organize the tests using `describe()` (try running jest with the --verbose flag!) * Lay the groundwork for the next commits, which add significant functionality By expanding test coverage and adding more corner cases, it will help ensure that the forthcoming features don't inadvertently change default functionality. Signed-off-by: Brian Warner <brian@bdwarner.com>
This commit moves some things around in preparation for adding support for remediation commits. It does a number of things which will be used in subsequent commits: * Adds config file support for remediation commits, which are disabled by default. * Updates the DCO check function call to be aware of remediation commits. * Refactors signoff failure feedback into a single function, which will be subsequently expanded. * Update the snapshot to reflect the updated text. In addition, this commit updates the default feedback text. While testing I noted that there could be situations where amending a commit would not fix the issue (e.g., if the failing commit was not at the tip). Since both amend and rebase regenerate the hash and require a force push, I consolidated feedback for both into one common text. Signed-off-by: Brian Warner <brian@bdwarner.com>
This commit adds support for individual and third-party remediation commits. These are disabled
unless explicitly enabled by the end user to preserve the default, more conservative functionality.
* What is a remediation commit?
In brief, a remediation commit is a way to achieve DCO signoffs without amending the original
commit. Remediation commits were proposed upon feedback from open source communities that PRs were
failing or being abandoned because not everybody checks their code out to a local git environment.
In particular, the GitHub UI does not currently support amending a commit log, so PRs which failed
the Probot DCO check were being abandoned by committers. Remediation commits are proposed as an
improvement because they can be executed entirely within the GitHub web UI, while still being a part
of a project's immutable and portable git history.
* Why would I use this when I can amend commits?
The process of amending commits works well for single contributors working alone in a branch, but it
has challenges when there are multiple contributors or when rebasing the branch would break others'
workflows. For example, `git rebase HEAD~10 --signoff` adds the author's signoff to all 10 prior
commits, whether they authored them or not.
The advantage of this approach is that it allows contributors to pinpoint the exact commits for
which they need to add a signoff, even if they've already been merged.
* How does a remediation commit work?
There are two types of remediation commits: individual and third-party.
For an individual remediation, the original author pushes a second commit with specific text in the
commit log indicating they are adding their Signed-off-by line retroactively. Only the original
author can provide an individual remedation.
For a third-party remediation, someone else pushes a second commit with specific text in the commit
log indicating they are adding their Signed-off-by line in place of the author. This is intended for
situations where the original author is not available and where another can make the DCO commitment
in their place (e.g., an employer).
* Why is this not turned on by default?
The default Probot DCO behavior mirrors the workflow of the Linux kernel, where an author who
forgets a DCO signoff must amend their commits and resubmit them. This is a well-known process and
a broadly adopted requirement, and the intent is not to change how Probot works for projects that
have adopted it under these expectations.
* How do I enable this?
Projects can enable remediation commits on a repo-by-repo basis by adding a `dco.yml` config file in
their `/.github` directory with the following lines:
allowRemediationCommits:
individual: true
thirdParty: true
Note that individual remediations must be enabled for thirdParty remediations to work.
* When should I enable this?
You should have a conversation with your project leadership to determine "if remediation commits are
sufficient to be considered a DCO Sign-off." There may be different answers for different projects.
Contact your legal counsel if you aren't sure what this means.
* How does a contributor know what remediation text to use?
Much as the DCO signoff text is standardized ("Signed-off-by: Name <email>"), the individual and
third-party remediations are also intended to be standardized. This will allow other tools to search
for the text and come to the same conclusion whether a commit has been correctly signed off. When
the check runs, it will show the exact text the author or a third-party will need to copy into the
commit message to constitute a valid remediation commit.
In general, the standard signoff texts are:
Default: Signed-off-by: Brian Warner <brian@bdwarner.com>
Individual: I, Brian Warner <brian@bdwarner.com>, hereby add my Signed-off-by to this commit: <SHA>
Third-party: On behalf of Not Brian <notbrian@notbdwarner.com>, I, Brian Warner <brian@bdwarner.com>, hereby add my Signed-off-by to this commit: <SHA>
---
This commit includes the following:
* Support for individual remediation commits
* Support for third-party remediation commits
* Tests that verify the new behavior
* Tests that verify the default behavior is unchanged when remediation commits are enabled
* An additional warning about how rebasing can break workflows if remediation is available
Signed-off-by: Brian Warner <brian@bdwarner.com>
Add a brief description of what remediation commits do, and how they differ from the default mode of operation. Signed-off-by: Brian Warner <brian@bdwarner.com>
Signed-off-by: Brian Warner <brian@bdwarner.com>
Signed-off-by: Brian Warner <brian@bdwarner.com>
| allowRemediationCommits: | ||
| individual: true | ||
| thirdParty: true | ||
| ``` |
There was a problem hiding this comment.
Can we add instructions on how to add remediation commits using git and the GitHub Web UI? I know you add instructions to the check runs which is great, but I think we should document it here as well, what do you think?
If I understand it correctly, ideally remediation commits would be empty commits with the correct messages, but there is no way to add empty commits using the GitHub Web UI as far as I know.
| allowRemediationCommits: | ||
| individual: true | ||
| thirdParty: true |
There was a problem hiding this comment.
I think setting thirdParty: true will set individual: true implicitly, setting both is no different to just setting thirdParty: true
https://github.com/brianwarner/dco/blob/eb53e5d32ce5d48b1467a581ed0ea404dc0a349c/index.js#L186
If that's the case, how about we change the setting to
allowRemediationCommits: individual # or: thirdParty
|
|
||
| let rebaseWarning = '' | ||
|
|
||
| if (allowRemediationCommits.individual || allowRemediationCommits.thirdparty) { |
There was a problem hiding this comment.
| if (allowRemediationCommits.individual || allowRemediationCommits.thirdparty) { | |
| if (allowRemediationCommits.individual || allowRemediationCommits.thirdParty) { |
There was a problem hiding this comment.
I caught this as I was adding more tests. No need to change the current PR, let's merge this into a local branch (not master) then I'll send a PR with my changes against it
| // Draft the magic DCO remediation commit text for the author | ||
| // returnMessage = returnMessage + `Retroactive-signed-off-by: ${commit.author} <${commit.email}> ${commit.sha}\n` | ||
| returnMessage = returnMessage + `On behalf of ${commit.author} <${commit.email}>, I, YOUR_NAME <YOUR_EMAIL>, hereby add my Signed-off-by to this commit: ${commit.sha}\n` | ||
| console.log(commit.sha) |
There was a problem hiding this comment.
I asume this is a leftover console.log()? I'll remove it in my follow up PR
Signed-off-by: Brian Warner <brian@bdwarner.com>
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.ymlfile, 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 --verbosenow 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 --amendis 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