Page MenuHomePhabricator

[land-bot] Add helper script for checking if a Differential diff was authored by a trusted source

Authored by jasonbcox on Sep 10 2020, 23:26.


Group Reviewers
Restricted Project

Currently, automated commits are generated, tested, and pushed by the same script.
This has obvious separation of responsibility concerns. We have a land bot that can land
human-authored patches, but it makes heavy assumptions about the code review process.

This patch is the first step to giving automated patch support to the land bot. Since automated
patches do not go through the code review flow, we need some other way to keep track of these
patches. We can leverage Differential's existing "Diff" framework (not to be confused with the
colloqual term "diff" we often use when we are actually talking about Revisions).

A Differential Diff has a patch, a unique ID, and an author. Differential acts as the
permissioned central repository for these patches to ensure the author information can
be trusted. The unique ID can be used for automated patch generators to call the land bot.
The land bot fetches the Diff using the ID, validates the author information, and then lands
the patch.

This script is the author validation piece. It will soon be used by other landbot machinary
to complete the process described above.

Test Plan
ninja check-source-control-tools
CONDUIT_TOKEN=<token> ./ 23430    # this will fail, but you can verify it's comparing against the author field

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 12708
Build 25508: Build Diffbuild-without-wallet · build-clang-10 · build-diff · build-clang-tidy
Build 25507: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Sep 11 2020, 01:34
deadalnix added a subscriber: deadalnix.

The description doesn't really explain the problem that this solves or why this approach. For instance why do we need to send the patch to phabricator to then redownload it at the other end? That seems to provide zero value over transferring the diff right away.


I'm not sure what this is doing, but certainly not checking the diff. I assume it is checking that a diff is trusted. You really need to work on giving things proper names.

This revision now requires changes to proceed.Sep 11 2020, 01:34

Since automated diffs do not need a build queue at this time, the value of completing this design is questionable.