search

TraceMachina / nativelink

NativeLink is an open source high-performance build cache and remote execution server, compatible with Bazel, Buck2, Reclient, and other RBE-compatible build systems. It offers drastically faster builds, reduced test flakiness, and significant infrastructure cost savings.
https://nativelink.com
Apache License 2.0
660 stars 75 forks source link

Reviewable reviews with an LGTM should make it look like "accepted" to GitHub #831

Open aaronmondal opened 3 months ago

aaronmondal commented 3 months ago

At the moment it's possible to merge commits where a reviewer left an LGTM stamp in Reviewable.

This isn't viewed as "accepted" by GitHub, leading to false positives on the OpenSSF scorecard check which verifies that every commit on main is reviewed.

For instance: https://github.com/TraceMachina/nativelink/pull/830

cc: @allada @zbirenbaum

allada commented 3 months ago

I don't understand, Zach left an LGTM, which is why it was able to be merged.

aaronmondal commented 3 months ago

Yeah this is confusing. It's an LGTM in reviewable, but a comment on github. Usually, when you give an LGTM in reviewable the github ui displays it automatically as "approval". In this case it's only a "comment".

For instance, in this case the LGTM correctly triggered a GitHub "approval": https://github.com/TraceMachina/nativelink/pull/836#pullrequestreview-1983739992

allada commented 3 months ago

The reviewable UI is the source of truth. We have a bunch of custom javascript that controls weather a PR is approved or not. Github does not give nearly as much flexibility, so we really need users to use Reviewable to get this data.

aaronmondal commented 3 months ago

The reviewable UI is the source of truth.

Yes. I don't want that to change. The only thing that I think we should adjust is that if the custom JS in reviewable decides that a PR is approved in reviewable, that the GitHub UI also marks it as approved (i.e. green approval mark instead of grey comment mark). This shouldn't influence whether a PR becomes "mergeable" since the Reviewable check controls this anyways.

This shouldn't change the workflow or any review-related permissions - the only thing it changes is that we make sure that the OSSF badge doesn't bug out because the LGTM from reviewable didn't propagate to "accepted" in GitHub.

allada commented 3 months ago

I think the issue you are referencing is that in reviewable we don't require every file to be marked "reviewed" by the reviewer, we only require that an LGTM stamp is there. These are propagating when the reviewer actually marks each file.

We could change this, but it'll but a bit more burden on the reviewer and the author will not be able to merge if they force push after an LGTM.

aaronmondal commented 3 months ago

Ah yes that seems to be it.

Is it possible to automatically mark all files as "reviewed" when someone gives an LGTM? This way we'd get the "approved" in GitHub and I think the OSSF action would stay happy even if the branch is force-pushed afterwards since it only checks for the existence of any github approval.

If we can get reviewable to do this I think that would fix the issue without putting additional burden on reviewers/authors.

allada commented 3 months ago

I'd rather go the other way. I often leave file un-reviewed as a queue to come back to them on the second pass after I give an LGTM but with a blocking comment.