Page MenuHomePhabricator

When builds fail because the staging area was not pushed, make the cause clear
Open, LowPublic

Description

I just got another internal support request about this, so I figured I should probably raise a feature request here. I semi-frequently get internal support requests within my company asking why builds on code reviews have failed. We put a nice big warning on them:

pasted_file (99×887 px, 19 KB)

We wrote this warning when people used to have outdated versions of Arcanist that didn't support pushing to staging repositories at all.

The issue now is often that users don't have the SSH agent configured correctly, because they clone over HTTPS (and the staging repository is SSH). As per Q264, I'm looking for a way to make "arc diff" fail if pushing to the staging repository fails. This is because if pushing to the staging repository fails, then the build for the diff will always fail because it can't get a working copy. Having to track down why the build is failing at this step costs more time than if "arc diff" just failed to push to the staging repository in the first place.

The other outcome of this is that developers get used to seeing the build fail on their diffs and ignore it (because none of the "arc diff"s they do result in a working build). I don't get any visibility over when developers are doing this, but it has the consequence that integration tests won't be running for those developers when they're submitting code.

Event Timeline

epriestley renamed this task from Allow repositories to require a successful push to the staging repository to When builds fail because the staging area was not pushed, make the cause clear.Jan 7 2016, 2:37 AM
epriestley triaged this task as Low priority.

We have patched around this issue locally with:

diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php
index 8a3fa6f..2530c9c 100644
--- a/src/workflow/ArcanistDiffWorkflow.php
+++ b/src/workflow/ArcanistDiffWorkflow.php
@@ -2731,9 +2731,16 @@ EOTEXT
       $tag);
 
     if ($err) {
-      $this->writeWarn(
+      $this->console->writeOut(
+        "<bg:red>** %s **</bg> %s\n",
         pht('STAGING FAILED'),
-        pht('Unable to push changes to the staging area.'));
+        pht(
+          'Unable to push changes to the staging area.  Make sure you have an SSH '.
+          'key loaded and PuTTY running.  Review the error above for more details.'));
+      throw new ArcanistUsageException(
+        pht(
+          'Unable to push changes to the staging area.  This must succeed in order '.
+          'for diff submission to continue.'));
     } else {
       $this->writeOkay(
         pht('STAGING PUSHED'),

I actually think failing universally like that is probably fine (you can --skip-staging anyway, as long as the error tells you about it) but the UI behavior in Phabricator itself is frequently nonsensical (we basically guess that the staging area probably exists and proceed blindly).

You mean from a Harbormaster Lease Working Copy perspective or a Land Revision perspective (when it comes to nonsensical UI).

After D15424, arc fails when failing to push the staging area.

After D15427, this should be pretty clear for "Land Revision".

This is still not particularly clear for Harbormaster builds, because they don't know how to surface Drydock VCS errors. You can find the error if you dig, but it's not obvious.

eadler added a project: Restricted Project.Aug 5 2016, 4:44 PM