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

DCO bot fails when checking commits from accepted change #102

Closed dpordomingo closed 5 years ago

dpordomingo commented 5 years ago

When using the GH interface to accept a change DCO bot rejects the automatically generated commit even when the Signed-off-by message was correct.

image

In my opinion, Co-Authored-By message (automatically introduced by GitHub UI) should be ignored.

hiimbex commented 5 years ago

@dpordomingo is the PR public/can you link it? If not can you show the DCO's status/message on that commit?

dpordomingo commented 5 years ago

Hi, many thanks for reviewing this.

This PR: https://github.com/src-d/landing/pull/350 Contains this commit https://github.com/src-d/landing/pull/350/commits/9b406acd073c2e8d35419b453f1b9f4f41714793, rejected by DCO bot, but its commit message is:

Author: David <rizome.es@gmail.com>
Commit: GitHub <noreply@github.com>

    Update README.md

    Signed-off-by: David Pordomingo <david.pordomingo.f@gmail.com>

    Co-Authored-By: dpordomingo <david.pordomingo.f@gmail.com>

You can compare it with this other PR: https://github.com/src-d/landing/pull/340 Containing this commit https://github.com/src-d/landing/pull/340/commits/ac64462e6c467c7cc2d22da73b8d4331339937b5, approved by DCO bot, with the following commit message:

Author: David Pordomingo <David.Pordomingo.F@gmail.com>
Commit: David Pordomingo <David.Pordomingo.F@gmail.com>

    Fix non-https links

    Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
hiimbex commented 5 years ago

Actually the issue is not with the co-authored by line, but the actual sign-off. If you look at the details of the check run (https://github.com/src-d/landing/pull/350/checks?check_run_id=52447545), you'll see:

Commit sha: 9b406ac, Author: David, Committer: GitHub; Expected "David rizome.es@gmail.com", but got "David Pordomingo david.pordomingo.f@gmail.com".

You can see this in the details you provided as well:

Author: David rizome.es@gmail.com Commit: GitHub noreply@github.com

vs

Author: David Pordomingo David.Pordomingo.F@gmail.com Commit: David Pordomingo David.Pordomingo.F@gmail.com

I think this may be due to how suggested changes works in which "GitHub" is credited as the committer and "David" aka rizome (not you) is credited as the author. In either case the sign-off does not match David aka rizome, who introduced the suggested change.

I'm not sure whether the suggested change commit credit should go to the suggestor or the one who accepts the suggestion. However, for the time being, the information GitHub gives us on that PR only tells us about rizome and that the commit was maybe in the GitHub UI, so we expect rizome's sign-off rather than yours.

GitHub
[RFC] Update README.md by dpordomingo · Pull Request #350 · src-d/landing
Do not merge; acts as an example for probot/dco#102
hiimbex commented 5 years ago

To be clear, the DCO currently checks for a sign-off on only the first Signed-off-by line of a commit. It compares that name and email to match either the committer or the author. If a match is not found, it will fail. In this case the signed-off-by line contained David Pordomingo <david.pordomingo.f@gmail.com>, however the author and committer were Author: David <rizome.es@gmail.com>; Commit: GitHub <noreply@github.com> so there was no match.

dpordomingo commented 5 years ago

imho DCO bot should be compatible with GitHub workflow, so it should avoid rejecting commits that are introduced by a normal GitHub workflow and that can not be modified by the end user.

I wonder if it would be a good idea to ignore commits that came from GitHub <noreply@github.com> (which are signed with the GitHub GPG key) until GitHub is able to properly sign-off accepted suggestions, or the DCO bot is able to validate those ones.

image

PS. In this other PR #14, it was also modified DCO bot to avoid checking for sig-off message on merges.

marnovo commented 5 years ago

@dpordomingo has a very good point here. Otherwise it becomes more of a hassle than a convenience, plus all the false positives…

hiimbex commented 5 years ago

PRs welcome; however, in this specific instance, I do think that fault is the awkward GitHub flow of suggested changes. Feel free to contact support if you'd like to see that fixed.

dpordomingo commented 5 years ago

Thanks @hiimbex for your time. Should I understand that you see reasonable the feature of ignoring commits that came from GitHub noreply@github.com, so you would accept a PR implementing it? If so, I'd give it a try.

hiimbex commented 5 years ago

Sorry, I wasn't very clear. I was suggesting we could support the Co-authored-by line as a sign-off, (opt in).

I'm pretty open to a variety of new features as long as they are opt-in via configs, so that the app can fit as many people's use cases as possible. Personally, I don't think we should completely ignore commits that happen through the GitHub UI, there's a lot of other scenarios in which I still think it reasonable to expect a sign-off; however, I'm still open to a PR for that as long as it's hyper clear what is happening and what enabling the config option does (through naming/docs).

stale[bot] commented 5 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?