Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DCO bot fails when checking commits from accepted change #102

Open
dpordomingo opened this Issue Jan 17, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@dpordomingo
Copy link

dpordomingo commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member

hiimbex commented Jan 20, 2019

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

@dpordomingo

This comment has been minimized.

Copy link
Author

dpordomingo commented Jan 21, 2019

Hi, many thanks for reviewing this.

This PR: src-d/landing#350
Contains this commit src-d/landing@9b406ac, 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: src-d/landing#340
Containing this commit src-d/landing@ac64462, 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

This comment has been minimized.

Copy link
Member

hiimbex commented Jan 21, 2019

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
Do not merge; acts as an example for #102
@hiimbex

This comment has been minimized.

Copy link
Member

hiimbex commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Author

dpordomingo commented Jan 29, 2019

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

This comment has been minimized.

Copy link

marnovo commented Feb 21, 2019

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

@hiimbex

This comment has been minimized.

Copy link
Member

hiimbex commented Mar 3, 2019

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

This comment has been minimized.

Copy link
Author

dpordomingo commented Mar 3, 2019

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

This comment has been minimized.

Copy link
Member

hiimbex commented Mar 7, 2019

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.