search

alexellis / derek

Reduce maintainer fatigue by automating GitHub
https://github.com/alexellis/derek/blob/master/USER_GUIDE.md
MIT License
806 stars 72 forks source link

Revisit: GitHub Checks / Commit statuses #95

Closed alexellis closed 4 years ago

alexellis commented 5 years ago

Expected Behaviour

We looked into this before in a previous issue, but I think we could leverage GitHub Checks or commit statuses to give feedback on the DCO and other things Derek checks.

https://github.com/alexellis/derek/issues/35

Current Behaviour

We post a label and a comment when one of the commits in a PR isn't signed off, but this still allows merging through the UI and some newer contributors don't understand that every commit needs to be signed-off-by - not just the last one.

Possible Solution

Prototype working with a commit status / check where one of several commits in a PR is detected as invalid to see if this can fail the whole check. The feedback given before by @stealthybox said only the final commit in a series determines the pass/fail of a PR (but we should check again)

martindekov commented 5 years ago

I will start researching how checks API work and will post the general way I will implement this and will wait for approval. I hope this sounds ok

alexellis commented 5 years ago

Sounds good 👍 - I'd advise looking into how we did this for OpenFaaS Cloud as a starting point.

martindekov commented 5 years ago

Ah yeah... thanks for pointing that out ^

martindekov commented 5 years ago

So looked into that, I see adding commit to already existing PR gives synchronize event. I think we should add this to the events which are handled and check the body for every event of that type.

It is different scenario, unlike Travis which builds your latest commit with all the changes before it, here we want to check every commit.

The synchronize event is triggered on every commit, no matter the PR, which is problem in a way I need to make sure the commit is in the proper PR, will see how to figure it out, not sure how big problem this might be.

Otherwise the plan is:

  1. Add synchronize event support to already existing events.
  2. For pull_request event put checks along with label and comment where those are added
  3. Send check with DCO name along with failed / completed status upon DCO present / non-present and short description like no-dco you have unsigned commit, since the comment is already descriptive enough

Hope this sounds good ^ ?

alexellis commented 5 years ago

So looked into that, I see adding commit to already existing PR gives synchronize event.

Which PR is that?

It is different scenario, unlike Travis which builds your latest commit with all the changes before it, here we want to check every commit.

So what if we experiment by setting a status for every commit it asks us to check and see what happens? Does it roll up the failed commit statuses in the PR to fail it a a whole?

For point 3 - keeping things brief sounds good.

How about just prototyping the absolute minimum amount of code to prove whether this works or not? You will need to add the Checks API to your GitHub App's permissions in addition to the Statuses API which is has already.

martindekov commented 5 years ago

Which PR is that?

Tested by opening PR with single commit in own project and then push another commit to the PR, the event triggered was synchronize, what we support currently is issue_comment and pull_request. Which makes me think if someone adds commit to already existing PR without signing it I think derek will not apply no-dco label currently. Correct me if I am wrong.

So what if we experiment by setting a status for every commit it asks us to check and see what happens? Does it roll up the failed commit statuses in the PR to fail it a a whole?

Yeah totally, but we can do that, but if I decide to add another commit to the existing PR it won't count that since the event triggered is not pull_request but rather synchronize, which is not supported

How about just prototyping the absolute minimum amount of code to prove whether this works or not?

Sounds like a good proposal. With minimum amount of code I can trigger the check on pull_request only to get all commits from that event and add checks for every one of them. Will start doing that and will open WIP PR with examples on the progress.

martindekov commented 5 years ago

I saw discussion going around in Derek, and though since I wanted proposals on the check content maybe I can make attributes configurable (title description summary etc.) So maybe we would like our check to be NO-DCO but moby would like to name it Unsigned-commit present or something like that. WDYT ?

cc @rgee0 @burtonr

martindekov commented 4 years ago

I will finish this one up after the cli as I already have opened PR for it.

alexellis commented 4 years ago

I think we dropped the ball on this one, but I can't remember if it was because the solution was non-optimal or because we got distracted. Do you remember @martindekov?

martindekov commented 4 years ago

I remember now I was doing cli change and left the change tested not fully e2e. Fixed that also re-based the commit it works as it should. All scenarios are covered in the example PRs opened here: https://github.com/martindekov/push2/pulls

Also re-based the change and fixed some corner cases which were left from the previous implementation.