[land-bot] Ensure changes to the land bot script do not modify its execution in-flight
ClosedPublic
Actions

Authored by jasonbcox on May 8 2020, 23:48.

Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING702095f647a4: [land-bot] Ensure changes to the land bot script do not modify its execution in…
rABC702095f647a4: [land-bot] Ensure changes to the land bot script do not modify its execution in…

Summary

I encountered this issue while testing locally. Even normal errors can turn
into very bizarre behavior that are impossible to comprehend when this is triggered.

There is an alternative approach where a wrapper script copies land-patch.sh and runs the copy,
but this approach is nearly as easy to maintain and does not require that developers know that
running land-patch.sh directly can cause debugging nightmares.

Test Plan

It's a bit hard to trigger this consistently, but for sanity:

CONDUIT_TOKEN=<token> ./land-patch --dry-run -r Dxxxx

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.May 8 2020, 23:48

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 8 2020, 23:48

jasonbcox requested review of this revision.May 8 2020, 23:48

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Harbormaster completed remote builds in B10669: Diff 19856.May 8 2020, 23:54

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

I have no problem with this per se now that it is done, but you got to wonder how that became a problem worth fixing in the first place. Why not run these scripts from master to begin with instead of using the version of the patch?

This revision is now accepted and ready to land.May 11 2020, 13:53

In D6018#147472, @deadalnix wrote:

I have no problem with this per se now that it is done, but you got to wonder how that became a problem worth fixing in the first place. Why not run these scripts from master to begin with instead of using the version of the patch?

The script is being run from master. The issue becomes when autopatch modifies land-patch, we expect land-patch behavior to *not* change. On the flip side, if smoke-tests are updated, we *do* expect that behavior to change.

Closed by commit rABC702095f647a4: [land-bot] Ensure changes to the land bot script do not modify its execution in… (authored by jasonbcox). · Explain WhyMay 12 2020, 22:38

This revision was automatically updated to reflect the committed changes.

jasonbcox added a commit: rABC702095f647a4: [land-bot] Ensure changes to the land bot script do not modify its execution in….

jasonbcox added a commit: rSTAGING702095f647a4: [land-bot] Ensure changes to the land bot script do not modify its execution in….