Support native approvals

[prayerslayer]

So GitHub just got native approvals, which is great! We should support them once they are available in web hooks (currently they aren't, as usual).

21.12.2016 Reviews are available in web hooks and API (with a special header) of public Github. We can now start to incorporate them into Zappr.

• Only people with at least write access to the repo are allowed to submit reviews. Check if this is sufficient for our GHE purposes. If so, it renders the approval.from configuration useless. If not, we can't integrate with native approvals until Github broadens access.

• Zappr should count reviews that approve changes as approvals.

• Pull request opener needs to be implicitly counted as approver, because she can't make it explicit due to how PR reviews work.

• Zappr should count reviews that request changes as vetos.

• Zappr should ignore reviews that only make comments.

• Support for comments as a means of reviewing will be dropped as soon as we have coomplete GHE support for reviews.

• As reviews stay around even after one pushed new commits, Zappr needs to behave the same. This is a breaking change to the previous behavior were vetoes disappeared after new commits.

• How to deal with dismissals? It could be ok to do the following: As long as a veto is present, block PR. If it's dismissed, track it in our audit system and proceed normally. (ie 1 veto and 1 approval -> dismiss veto -> PR is mergable).

[prayerslayer]

Update on this:

Reviews are now supported in the web hooks 🎉 However in order to replace our existing workflow we also need "require reviews before merging" to be in the Branches API, which is not the case as of now.

[lukasniemeier-zalando]

But can we support "zappr approvals" inside the native reviews?

[ePaul]

I think Zappr's approval check could just accept/interpret a native approval as an alternative to a comment with 👍?

[prayerslayer]

@lukasniemeier-zalando: But can we support "zappr approvals" inside the native reviews?

Yes.

[prayerslayer]

So to give an update on this. Technically it's now possible to integrate native approvals into Zappr. However preliminary testing revealed some oddities about GH approvals, like this one:

Integrating with GH approvals is useless as long as this can happen.

[j0k3r]

This is a really bad behavior but it seems that they are aware of it.
https://twitter.com/GitHubHelp/status/806641222630703104

[prayerslayer]

Ahh, thanks for asking ❤️ So probably it will "just work" one day without prior announcement 😁

[ePaul]

@prayerslayer Not sure how the native approvals look in the API, but if you can get their timestamp (or their relative order in comparison to the pushes), we should still be able to handle a native approval just as a ":+1:" comment, and thus use the existing approval verification logic?

[prayerslayer]

No, apparently there's no timestamp.

Also we would run into the situation where there's a GH approval saying everything's fine and Zappr saying that a review is missing... Not the best UX.

[ePaul]

Hmm, no timestamp, but a commit id. So we could compare that commit id with the pull request's head sha to see whether the approved commit was the latest one.

I agree that the UX is not the best, but the current state (the approvals are not considered at all) is even worse. (Possibly Zappr's message could be changed to say something like "There were commits after the last approval, please approve again" in this case.)

[lukasniemeier-zalando]

Maybe I am missing the point, but coming back to my comment #442 (comment) - isn't the "review" itself a "comment" (with a timestamp and all the bells)? So if I put a 👍 as my "review summary", can zappr count this as an approval correctly?

[prayerslayer]

isn't the "review" itself a "comment" (with a timestamp and all the bells)

It's not. It's a "review" which is different from a "review comment" which is different from an "issue comment" (=> which is what Zappr relies on).

So we could compare that commit id with the pull request's head sha to see whether the approved commit was the latest one.

Yeah I guess that would work. But I feel it's effort to fix something that shouldn't work that way in the first place. I'd rather sit it out, wait for GH to fix their review process and then do a clean transition, where a native approval "just works".

[mvalkon]

@prayerslayer @ePaul @lukasniemeier-zalando seems like GH has fixed this problem at least partially isaacs/github#783

If we can live with the preconditions to use this (master branch protection enabled with the options mentioned in the issue) then I think we could enable this. Any thoughts?

[ruudk]

Any news on this? I work on a lot of PR's for Skipper, and it's super annoying to have all my PR's always in orange state because zappr checks are not yet finalized.

[yousifalraheem]

So to give an update on this. Technically it's now possible to integrate native approvals into Zappr. However preliminary testing revealed some oddities about GH approvals, like this one:

Integrating with GH approvals is useless as long as this can happen.

Actually, Github allows you to dismiss PR reviews when new commits are pushed. You just have to configure it.

[red-avtovo]

I'm afraid to ask if there is any updates on that issue