dcoapp / app

GitHub App that enforces the Developer Certificate of Origin (DCO) on Pull Requests
https://github.com/apps/dco
ISC License
296 stars 73 forks source link

Adding a web link to remediate commits that are not signed off #170

Open gr2m opened 2 years ago

gr2m commented 2 years ago

This is a suggest to lower the barrier for creating remediation commits (depends on https://github.com/probot/dco/pull/147).

As of now, there is no simple way using the GitHub UI to add empty commits with the correct commit message body to sign off for previous commits.

I suggest to implement the following flow

  1. person submits a PR (e.g. fixing a typo using GitHub's UI) without signing their commit
  2. The DCO app adds a comment prompting the user to sign, providing a link to do so
  3. The link redirects to a custom route of the DCO app with a reference to the pull request
  4. The user is asked to authenticate using GitHub
  5. The app creates an empty commit with the correct commit body to sign off on the previous commit, authenticated as the user.
  6. The user gets redirected to the pull request
  7. Because of the newly added remediation commit, DCO can now set the status to success
  8. Bonus: the comment from the DCO app gets hidden as outdated

This could be an opt-in feature.

Note: Adding the comment will require an additional permission which needs to be approved by owners of all installations. But we can add the same link to the check runs we create, where we already document how to fix commits that are not signed off. It will be the simplest solution of all.

What do you think @ryjones @brianwarner @ashleywolf?

Willmish commented 1 year ago

Bumping this up, as I couldn't find any other Issues open/closed on remediation commits.

Could there be a section in the docs added that explains the concept a bit more/example of how to add a remediation commit?

Currently only info availble on it is this: https://github.com/dcoapp/app#individual-remediation-commit-support

However the worklow itself (on failed DCO check) states that the preferred way is by adding a remediation commit:

Preferred method: Commit author adds a DCO remediation commit

A DCO Remediation Commit contains special text in the commit message that applies a missing Signed-off-by line in a subsequent commit. The primary benefit of this method is that the project’s history does not change, and there is no risk of breaking someone else’s work.

These authors can unblock this PR by adding a new commit to this branch with the following text in their commit message:

If this is the preferred method, shouldn't there be a more detailed guide / information on how remediation commits should look like?

ryjones commented 1 year ago

@Willmish How would the README be better written?

Willmish commented 1 year ago

@ryjones - I would suggest either to add the messages that appear in the workflow log when DCO fails (explaining remediation Commit is preferred way of fixing vs Rebasing) and give an example of such Remediation commit. Here are the workflow logs quoted: https://github.com/Green-Software-Foundation/carbon-aware-sdk/issues/309#issuecomment-1474083877

I could not find an alternative source that explains this after DCO error was fixed and passing, so could not refer back to these exact messages (Structure of a remediation commit, and description of why its the preferred method over rebasing). Maybe there is an external documentation explaining this (in GItHub docs?).

So my suggested change would be to, in the section https://github.com/dcoapp/app#individual-remediation-commit-support add the following:

ryjones commented 1 year ago

@Willmish so incorporate the content of the how it works section, as well as adding an example and explanation of remediation versus rebasing

Willmish commented 1 year ago

@ryjones Yup - in my opinion that would be ideal as it would add a reference point for explaining remediation commits, if ever needed to be pointed to in other discussions. This is already very well described when the actual workflow fails and provides a clear guide how to comply with DCO, but I think this message sitting in the repository itself would still be helfpul when there is no failed workflow log message to refer to :D .

Willmish commented 1 year ago

This comment https://github.com/dcoapp/app/pull/171#discussion_r749937389 by @gr2m also captures what I think the README / docs should include

brianwarner commented 1 year ago

@ryjones want to assign that to me?

ryjones commented 1 year ago

@brianwarner I can't - I have membership, but no commit bit

Willmish commented 1 year ago

Hi @brianwarner @ryjones - just bumping this up (I know this is not critical, doesn't want it to go stale)

ryjones commented 1 year ago

@gr2m may I have the permission to assign this issue to @brianwarner ?