nativelink
nativelink copied to clipboard
TraceMachina
Reviewable reviews with an LGTM should make it look like "accepted" to GitHub
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
I don't understand, Zach left an LGTM, which is why it was able to be merged.
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
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.
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.
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.
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.
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.