[land-bot] Cleanup unnecessary revision acceptable check
Needs RevisionPublic
Actions

Authored by jasonbcox on Sep 17 2020, 18:42.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Summary

I originally thought this was necessary to prevent patching unwanted
changes that could override further sanity checks. Unfortunately this
duplicates logic already done by arc land --hold. It turns out there is a
better way to do this such that we fully leverage arcanist without exposing
the land bot to malicious patches.

Depends on D7479

Test Plan

cmake -GNinja -DENABLE_SOURCE_CONTROL_TOOLS_TESTS=ON ..
ninja check-source-control-tools
CONDUIT_TOKEN=<token> ./land-revision.sh --dry-run Dxxxx

Diff Detail

Repository

rABC Bitcoin ABC

Branch

lb-rm-acceptance-check

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12784
Build 25641: Build Diff	build-diff · build-clang-tidy · build-without-wallet · build-clang-10
Build 25640: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Sep 17 2020, 18:42

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 17 2020, 18:42

jasonbcox requested review of this revision.Sep 17 2020, 18:42

Harbormaster completed remote builds in B12780: Diff 23564.Sep 17 2020, 19:05

Rebase

jasonbcox edited the test plan for this revision. (Show Details)Sep 17 2020, 21:45

Harbormaster completed remote builds in B12784: Diff 23570.Sep 17 2020, 22:02

deadalnix requested changes to this revision.Sep 18 2020, 00:50

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

contrib/source-control-tools/land-revision.sh
68	This is pretty bad. Either you want to allow patches to modify these script in a meaningful way - in which case it's all good - or you don't and in which case, you need to do this for every script, which is never going to work because it's fragile.

This revision now requires changes to proceed.Sep 18 2020, 00:50

jasonbcox added inline comments.Sep 21 2020, 17:25

contrib/source-control-tools/land-revision.sh
68	You don't need to do this for any other script. It's only necessary here to ensure the revision was actually accepted. Any changes that impact the behavior of the pipeline after this point would have been reviewed and accepted. The current design has a similar restriction in that changes to `check-revision-accepted.sh` do not affect the pipeline until the change is landed. The tradeoff here is: Current solution - More complexity and code. This patch - Slightly increased scope on the logic in this pipeline that does not get updated until the patch is successfully landed. I do not expect this logic to change often, though.