Page MenuHomePhabricator

Add a script for building and pushing automated commits
ClosedPublic

Authored by jasonbcox on Jan 22 2020, 23:55.

Details

Summary

This script provides a similar function to contrib/teamcity/build-configurations.sh and allows us to define various
pipelines that a bot can and is expected to run, given some trigger (such as a schedule). This initial version of the script
supports update-chainparams only for now, but others will soon be added.

To prep for this script's deployment, I've done the following:

  • Setup a machine to run bitcoind instances on mainnet and testnet where this script can be run.
  • Created a bot user 'abc-bot' on Phabricator.
  • Added Herald rules to allow abc-bot to push changes upstream without code review.

The script will be scheduled in Teamcity to run update-chainparams once per week.

Test Plan
git ch -b testbranch  # we assume we're not writing changes directly on master
./automated-commits.sh  # errors as expected

bitcoind &
PID1=$!
bitcoind --testnet &
PID2=$!
DIFF_NAME=update-chainparams ./automated-commits.sh  # succeeds, but as a dry-run

sudo kill $PID2
git ch testbranch
DIFF_NAME=update-chainparams ./automated-commits.sh  # fails, leaving uncommitted changes

git stash
git ch testbranch
git stash pop         # test with dirty source tree
bitcoind --testnet &
PID2=$!
DIFF_NAME=update-chainparams ./automated-commits.sh  # succeeds, but as a dry-run
sudo kill $PID1 $PID2

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Jan 22 2020, 23:55
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 22 2020, 23:55

Does this make diffs automatically?

Does this make diffs automatically?

Not phab diffs, but if you consider a git commit being a diff, then yes.

jasonbcox updated this revision to Diff 15916.Jan 30 2020, 23:14

automated diff -> automated commit since 'diff' is ambiguous

jasonbcox retitled this revision from Add a script for building and pushing automated diffs to Add a script for building and pushing automated commits.Jan 30 2020, 23:16
jasonbcox edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Feb 4 2020, 18:27
Fabien added a subscriber: Fabien.

Mostly nit picking

contrib/source-control-tools/automated-commits.sh
12 ↗(On Diff #15916)

Nit: consistency between $DIFF_NAME and ${DIFF_NAME}

13 ↗(On Diff #15916)

DIFF_NAME is a not a meaningful name, since it's not a diff (in the sense of phabricator) and you're not setting a name.

29 ↗(On Diff #15916)

Dito

58 ↗(On Diff #15916)

IDK how complicated it could be, but it would be nice to run the linters before committing to prevent the bot from adding some formatting errors that would need to be fixed by users.

This revision now requires changes to proceed.Feb 4 2020, 18:27
jasonbcox updated this revision to Diff 16020.Feb 4 2020, 23:34

DIFF_NAME -> COMMIT_TYPE

IDK how complicated it could be, but it would be nice to run the linters before committing to prevent the bot from adding some formatting errors that would need to be fixed by users.

As we discussed offline, I think running the linter automatically hides underlying issues with our codegen. So instead, we can run
the linter without applying the patches and check for any issues. If there are, this build should fail, indicating that the
codegen needs fixing. The now updated diff does just that.

jasonbcox updated this revision to Diff 16021.Feb 4 2020, 23:36

Fix parts of the new error message that didn't make sense.

deadalnix accepted this revision.Feb 5 2020, 01:00

This doesn't seems to me like this is building capital toward more automation, but this is something.

Fabien requested changes to this revision.Feb 5 2020, 15:44
Fabien added inline comments.
contrib/source-control-tools/automated-commits.sh
62 ↗(On Diff #16021)

This is brittle. If the linter doesn't display "OKAY" then you can expect the return code to be non zero.

63 ↗(On Diff #16021)

Note that this will fail if there is an advice message. This is currently possible if you remove a boost include from the codebase for example, but is unlikely to happen in an automated commit.

64 ↗(On Diff #16021)

Print the linter output in this case, that would make it easier to debug should a false positive happen.

This revision now requires changes to proceed.Feb 5 2020, 15:44
jasonbcox added inline comments.Feb 5 2020, 23:54
contrib/source-control-tools/automated-commits.sh
62 ↗(On Diff #16021)

Surprisingly not true. The linter can give advice/warnings and still returns 0, which phabricator screams at during review, so I think the end result is the same as if there were linter errors. Checking for OKAY and also checking that OKAY is the only line seems to be the most robust way to ensure the autogen code is completing lint free.

63 ↗(On Diff #16021)

See above comment.

64 ↗(On Diff #16021)

It should be printed by the set -x at the top of the script when LINT_OUTPUT is set. Would you still prefer the extra redundancy?

Fabien added inline comments.Feb 6 2020, 07:25
contrib/source-control-tools/automated-commits.sh
62 ↗(On Diff #16021)

After a quick test:
Errors return code 2; warnings return code 1; autofixes and advices (and disabled of course) return code 0.
Errors and warnings both prevent the OKAY line to be displayed.

So in the end you can have the exact same check by looking the return code rather than grepping for OKAY, with some benefit:

  • It is robust against arcanist output changes,
  • It won't fail if there is the word OKAY in the linted code (mitigated by the line count).
63 ↗(On Diff #16021)

Actually you can also prevent this failure by passing the --severity=autofix option to prevent the linter from outputting advices.

64 ↗(On Diff #16021)

It's fine i just forgot +x does variable expansion.

Fabien added inline comments.Feb 8 2020, 17:04
contrib/source-control-tools/automated-commits.sh
63 ↗(On Diff #16021)

After looking at D5227, it looks reasonable to avoid prevent linter advices, as the phabricator message (Linter found an issue) is confusing.

jasonbcox edited the test plan for this revision. (Show Details)Feb 10 2020, 17:48
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox updated this revision to Diff 16224.Feb 10 2020, 17:51

OKAY -> exit code check
Also expanded the test plan

Fabien accepted this revision.Feb 10 2020, 17:54
This revision is now accepted and ready to land.Feb 10 2020, 17:54