Add fail check for dco - Githubissues

martindekov commented 5 years ago

Adding check which determines whether or not there are existing commits which are unsigned. The check is determined by the no-dco label.

Signed-off-by: Martin Dekov (VMware) mdekov@vmware.com

Adding succesful/failure check when unsigned commits are present in the PR

Description

Adding check which is successful by default and when no-dco label is applied the check fails, when the commits are signed the check is again successful

I think the allow merge if all checks pass would be turned on in the GitHub UI. I have red something about required status checks or something

Motivation and Context

[x] I have raised an issue to propose this change (required)

Fixes #95

Also suggestions on the text, the content of the titles body etc. Would be appreciated!

How Has This Been Tested?

Tested in repository here

Note: I have local minikube instance exported with ngrok so ping me if the robot does not work

Verbose diagram:

untitled diagram

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I've read the CONTRIBUTION guide
[x] I have signed-off my commits with git commit -s
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

alexellis commented 4 years ago

@martindekov I was chatting with Packet here https://github.com/tinkerbell/tink/issues/202 and recently accidentally merged two PRs that had a missing DCO.

Do you think this approach still makes sense, are there any unresolved issues, or if merged, would this give us the result we want?

martindekov commented 4 years ago

Well the logic is pretty straight forward, not sure why I've put WIP in the title this was some time ago 😄 I will re-base, test e2e and ping you for merge 👍

martindekov commented 4 years ago

All pull requests: https://github.com/martindekov/push2/pulls reflect the testing on the feature

martindekov commented 4 years ago

Feel free to double check and merge :+1:

alexellis commented 4 years ago

The term "Unsigned" may be problematic and confusing vs. GPG signing. Could you suggest some alternatives?

alexellis commented 4 years ago

Can the check's long text link to the contributing guide and to the Commits Page for the PR?

alexellis commented 4 years ago

@martindekov please could you test what happens if a repo has DCO turned off in their repo settings?

martindekov commented 4 years ago

For:

@martindekov please could you test what happens if a repo has DCO turned off in their repo settings?

Already tested and the check is turned off if the dco feature is turned off or not mentioned. For the Unsigned term for failed it can be: One or more of your commits are unsigned -> All your commits need to be signed for the [Developer Certificate of Origin](https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin)

alexellis commented 4 years ago

We already have a URL for the CONTRIBUTING guide, so let's try this?

One or more of the commits in this Pull Request are not signed-off.

alexellis commented 4 years ago

@martindekov could you update the message please? I'll get this released and we'll be able to start using it live.

martindekov commented 4 years ago

Yep will update it today and ping you :+1:

martindekov commented 4 years ago

@alexellis Now One or more of your commits are unsigned -> One or more of the commits in this Pull Request are not signed-off. as per your suggestion. Feel free to merge :+1:

alexellis commented 4 years ago

Once merged and released, this didn't actually work.

derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:31:24 Version: 0.18.10  SHA: 80b6976c106370a7081b2f8e9099a6ea9638e1f3
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:31:24 Timeouts: read: 15s, write: 15s hard: 0s.
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:31:24 Listening on port: 8080
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:31:24 Writing lock-file to: /tmp/.lock
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:31:24 Metrics listening on port: 8081
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:32:13 Forking fprocess.
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:32:13 Query  
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:32:13 Path  /
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:32:14 stderr: time="2020-07-23T13:32:14Z" level=fatal msg="Error while creating successful DCO check: Error while creating successful DCO check: POST https://api.github.com/repos/openfaas/org-tester/check-runs: 403 Resource not accessible by integration []"
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:32:14 Success=false, Error=exit status 1
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:32:14 Out=

Seems like we need to update the docs and permissions for Derek to add checks to his access.

martindekov commented 4 years ago

Interesting ... let me check the permissions on my bot

alexellis commented 4 years ago

Also strange, was that the label for no-dco wasn't added, which it should have been, I think?

martindekov commented 4 years ago

The logic of dco check is above the logic of the dco label. So I suppose when we log.Fatal on error the process exits with 1 before dco label is applied. That's why label is not present.

alexellis commented 4 years ago

Think I found a bug here

https://github.com/openfaas/org-tester/pull/30

alexellis commented 4 years ago

It's not even adding labels anymore, the behaviour is missing commits without sign-off

alexellis commented 4 years ago

I'm now seeing:

derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:40:53 Out=
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:40:53 Forking fprocess.
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:40:53 Query  
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:40:53 Path  /
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:40:55 stderr: time="2020-07-23T13:40:55Z" level=fatal msg="Error getting commits for PR 30\nGET https://api.github.com/repos/openfaas/org-tester/pulls/30/commits: 401 Bad credentials []"
derek-0100.1.d9wi3eb6z7md@derek.openfaas.com    | 2020/07/23 13:40:55 Success=false, Error=exit status 1

What other permissions are required?

martindekov commented 4 years ago

I suppose Checks read and write. Not sure if by default the app has those as before Derek did not use check I think.

alexellis commented 4 years ago

It has those now for the OpenFaaS org, the PR I linked to above shows a bug, could you try to repro locally?

martindekov commented 4 years ago

Yeah will do now 👍

alexellis commented 4 years ago

Thanks, I'll set Derek back to 0.9.13 for users in the meantime

martindekov commented 4 years ago

Interesting: https://github.com/martindekov/push2/pull/41 Labels and dco check is applied/removed when it should. Could this be a configuration problem?

martindekov commented 4 years ago

and

alexellis / derek

Add fail check for dco #108

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist: