search

dcoapp / app

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

WIP: Report errors in a comment #33

Closed bkeepers closed 6 years ago

bkeepers commented 6 years ago

I was thinking more about the issues we've had with reporting errors to the users (#17, #18, #19, #27, #32), and started exploring a different pattern for it while on a plane the other day.

  1. If the DCO check succeeds, everything stays the same
  2. If the DCO check fails, the app comments with a friendly error explaining what the DCO is and how to fix the failure, along with a detailed summary of the status for each commit. The URL of this comment is used as the target_url for the commit status, so clicking on the failure will jump the user up to this comment.
  3. If the DCO check fails again on subsequent pushes, this comment is updated with the latest latest details instead of spamming the user with another comment.

This allows us to avoid building out a heavy web UI (#27) for a while longer, and I think will be a better experience for end-users because it generates a notification on the first failure.

cc @keavy @kdaigle @kytrinyx

bkeepers commented 6 years ago

Here's an example of the comment that app would make on failure:

:x: DCO check failed

This project uses the Developer Certificate of Origin (DCO), which is a lightweight way for you to certify that you wrote or otherwise have the right to submit the code you are contributing to this project.

If you agree with the DCO, add a Signed-off-by: You Name <your@email.com> line to each of your commits. You can easily add this with:

$ git rebase --signoff 488e5285

After adding the DCO signoff to your commits, you will be required to force push your changes to this branch.

$ git push --force
View status of each commit - ✔️ [488e5285](https://github.com/probot/dco/pull/33/commits/488e52853a0aafa0787ac461244722a969100292) - Merge commits do no require the sign-off - ❌ [488e5285](https://github.com/probot/dco/pull/33/commits/e32628f93118e670110782b1424ec2b34a949580) - The commit sign-off is missing - ✔️ [d2e63bfc](https://github.com/probot/dco/pull/33/commits/d2e63bfcb930c9b7a93366bf4905fe32b468dc4b) - Includes a valid sign-off
kytrinyx commented 6 years ago

This is incredibly helpful. If they force push and are still missing a sign-off, would the DCO bot post a new comment, or would it update the comment that is already there?

hiimbex commented 6 years ago

The current implementation would be a new comment, and I think that makes sense, since most people might not think to look at the old one. Maybe it could edit the contents of any previous DCO comments to be hidden under a details view:

View outdated DCO status all previous contents here

This way it wouldn't take up as much space on the PR.

kytrinyx commented 6 years ago

Maybe it could edit the contents of any previous DCO comments to be hidden under a details view

That would be a nice extra touch in terms of user experience.

bkeepers commented 6 years ago

The detailed status for each commit (which is collapsed by default) is the only thing that changes on push or force push, and the resolution is always the same (rebase with -s).

For that reason, I was thinking it would just edit the original comment to update the current state of the failure. That way, the user is introduced to why it is failing the first time, and gets basic information on resolving the failure, but they don't get spammed every time they push.

Back in the day before commit statuses, Travis CI would post a new comment every time you push, which got really annoying for other contributors.

…most people might not think to look at the old one

The target_url would point to the comment, so hopefully the failed status would be enough to remind them of the comment they saw earlier, and point them in that direction.

Does that make sense at all?

kytrinyx commented 6 years ago

The target_url would point to the comment, so hopefully the failed status would be enough to remind them of the comment they saw earlier, and point them in that direction.

Ah, yepp. Yes this makes total sense.

hiimbex commented 6 years ago

Ahh that makes so much more sense. I was thinking the commit sha would change but it wouldn't since it's the base. 👍 for just editing the original comment.

stale[bot] commented 6 years ago

Is this still relevant? If so, please comment with any updates or addition details.