Companion Update Flow might fail silently

[joao-paulo-parity]

This issue is related to the scenario in paritytech/polkadot#2610

Errors were found upon further inspection of the Google Cloud logs, therefore weakening the argument for paritytech/polkadot#2610 (comment) being in the right trail for that situation (it'd be part of another problem: #236).

This issue therefore lists findings about the current problems with the Companion Update Flow not related to how the build system currently manages concurrent merge requests, but instead centered around how failure is badly handled currently. While #236 and #250 can't be solved reliably in the short term, what we can do for the time being is post a comment when an error happens so that developers can resolve them manually.

Problems

Cloning might fail

In

parity-processbot/src/companion.rs

Line 65 in dee4d24

log::info!("Cloning repo.");

a git clone is attempted. It might fail, and it indeed failed in that situation, although the error didn't matter. Either way, failure is not handled currently, but it should be. From the logs:

fatal: destination path 'polkadot' already exists and is not an empty directory

Merge might be done on a outdated master branch

The merge master command does not fetch origin/master before attempting the merge, although it should, since it's only guaranteed to be up-to-date on a fresh clone (which might fail, as described above).

parity-processbot/src/companion.rs

Line 120 in dee4d24

log::info!("Merging master.");

It showed up in the logs:

{
  "textPayload": "[2021-03-16T12:04:05Z INFO  parity_processbot::companion] Merging master.\n",
},
{
  "textPayload": "Already up to date.\n",
}

Regardless of whether or not origin/master was indeed up-to-date with Github at that point in time, this measure still needs to be taken into consideration.

The bot fails silently

The logs shows that the cargo update command had failed:

{
  "textPayload": "Updating git repository `https://github.com/paritytech/substrate`\n",
},
{
  "textPayload": "Updating crates.io index\n",
},
{
  "textPayload": "error: no matching package named `sp-election-providers` found\n",
}

Due to the bugs mentioned before, no change was made to the repository, which meant there was nothing to commit

  "textPayload": "[2021-03-16T12:04:18Z INFO  parity_processbot::companion] Committing changes.\n",
},
{
  "textPayload": "On branch gav-abstract-payout-curve\n",
},
{
  "textPayload": "Your branch is up to date with 'temp/gav-abstract-payout-curve'.\n",
},

Even if those commands were properly checked for their exit status, as it currently is the bot would still fail silently with a single uninformative comment (as in paritytech/polkadot#2610 (comment)) because those errors are simply logged without posting a comment

parity-processbot/src/webhook.rs

Lines 1281 to 1284 in dee4d24

	log::info!(
	"Failed updating companion {}",
	comp_html_url
	);

Solution

• Properly check all commands for errors
• Make the flow more robust (e.g. handle the case where the repository was already cloned without blindly ignoring the error)
• When the update flow fails, post a comment for context