Page MenuHomePhabricator

Added a land bot script
AbandonedPublic

Authored by jasonbcox on Oct 2 2019, 00:25.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

This is the first step to enabling us to land changes more safely.
Currently, it's a toy script that rebases a diff on master, builds, and runs tests.
It's designed to error out immediately if any step of this process fails or prompts
for user input (as the intent is for this to be 100% automated in the near future).

Depends on D4213

Test Plan

This errors as expected for red diffs and completes (but doesn't push) for green diffs:

ABC_BUILD_NAME=build-without-wallet ./land-bot.sh --no-land -r Dxxxx

This was used to land D4196 successfully:

ABC_BUILD_NAME=build-default ./land-bot.sh -r D4196

Diff Detail

Repository
rABC Bitcoin ABC
Branch
land-bot
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7660
Build 13359: Bitcoin ABC Buildbot (legacy)
Build 13358: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Oct 3 2019, 20:28
Fabien added a subscriber: Fabien.

Should anything fail the script errors out immediately, but the user working tree may have been modified in between.
I think you should trap the error and restore the previous state to avoid confusing the user.

This revision now requires changes to proceed.Oct 3 2019, 20:28
  • Rebase on D4213 and take advantage of it to make this script more robust.
  • Cleanup the repo state, especially in the case of failures.
deadalnix requested changes to this revision.Oct 6 2019, 18:15
deadalnix added inline comments.
contrib/bots/land-bot.sh
6 ↗(On Diff #13357)

Looks like this would benefit from some utf-8

19 ↗(On Diff #13357)

This suggest this does more than one thing.

29 ↗(On Diff #13357)

I'd suggest not building the tree at all. Also, it's a bit strange that this is the responsibility of this script to do sanity check.

69 ↗(On Diff #13357)

The repository can be in all kind of fucked state. Semi applied patches, mid rebase or cherry-pick failure, etc...

This is not sufficient. Also make sure that you do do not assume that scripts are good citizen and won't trash the repository. Having them do their best is okay - though that may in fact hide errors. You need to be able to pickup the repository in any shape or form and make it work.

This revision now requires changes to proceed.Oct 6 2019, 18:15
contrib/bots/land-bot.sh
29 ↗(On Diff #13357)

This is an interesting point. Thinking about it some more, a rebasing bot could be separate from the land bot. The rebasing bot would need to build and run tests however, as it's modifying the source.

contrib/bots/land-bot.sh
29 ↗(On Diff #13357)

Thinking more while actually writing the code I realize the last part isn't true. Separate scripts that are stringed together like: rebase -> build -> land have no dependencies on each other but complete the goal much more cleanly.