Page MenuHomePhabricator
Create Task
Maniphest T10093

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

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.

Revisions and Commits

rARC Arcanist
D15426rARCccbaee585e1a When `arc` pushes to the staging area, tell Phabricator what we did
rP Phabricator
D15427rP2ddd78647b20 Improve "Land Revision" errors for issues related to staging areas
D15420rP19eee427adb3 Improve Drydock errors for empty commits and missing changes

Related Objects

Event Timeline

hach-que created this task.Jan 7 2016, 12:02 AM
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.
epriestley added projects: Harbormaster, Diffusion.
epriestley added a subscriber: epriestley.EditedJan 7 2016, 2:39 AM

Outside of Harbormaster, "Land Revision" is also impacted.

See also:

https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/storage/DifferentialDiff.php$485-490

hach-que added a comment.Jan 7 2016, 2:46 AM

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'),
epriestley added a comment.Jan 7 2016, 2:48 AM

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).

joshuaspence added a subscriber: joshuaspence.Jan 7 2016, 7:58 AM
cburroughs added a subscriber: cburroughs.Jan 7 2016, 2:51 PM
hach-que added a comment.Jan 7 2016, 8:42 PM

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

epriestley moved this task from Backlog to v3 on the Harbormaster board.Feb 26 2016, 3:06 PM
epriestley edited projects, added Harbormaster (v3); removed Harbormaster.
epriestley added a revision: D15420: Improve Drydock errors for empty commits and missing changes.Mar 6 2016, 11:37 PM
epriestley added a commit: rP19eee427adb3: Improve Drydock errors for empty commits and missing changes.Mar 7 2016, 1:29 PM
epriestley added a comment.Mar 7 2016, 2:55 PM

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

epriestley added a revision: D15426: When `arc` pushes to the staging area, tell Phabricator what we did.Mar 7 2016, 3:15 PM
epriestley added a commit: rARCccbaee585e1a: When `arc` pushes to the staging area, tell Phabricator what we did.Mar 7 2016, 3:24 PM
epriestley added a revision: D15427: Improve "Land Revision" errors for issues related to staging areas.Mar 7 2016, 4:49 PM
epriestley added a comment.Mar 7 2016, 5:27 PM

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.

epriestley added a commit: rP2ddd78647b20: Improve "Land Revision" errors for issues related to staging areas.Mar 7 2016, 8:46 PM
20after4 added a subscriber: 20after4.Apr 5 2016, 5:01 PM
eadler added a project: Restricted Project.Aug 5 2016, 4:44 PM
Herald added a subscriber: eadler. · View Herald TranscriptAug 5 2016, 4:44 PM
rabahmeradi mentioned this in Q480: Why Harbormaster build always fail for diffs by a certain user ? (Answer 455).Sep 2 2016, 12:13 PM
bgamari added a subscriber: bgamari.Oct 13 2016, 12:58 PM
mpickering added a project: Haskell.org.Dec 21 2016, 11:17 AM
epriestley mentioned this in D17189: Stop hardcoding the YACC error message.Jan 12 2017, 3:27 PM
epriestley mentioned this in D19261: Remove accidental sprintf injection in error reporting.Mar 26 2018, 9:18 PM
epriestley mentioned this in T13210: Plans: 2018 Week 41-44 Bonus Content.Oct 19 2018, 6:51 PM
epriestley mentioned this in T13216: 2018 Week 45-47 Bonus Content.Nov 6 2018, 12:50 PM
epriestley mentioned this in T13088: Plans: Harbormaster UI usability and interconnectedness.Nov 21 2018, 5:07 PM
epriestley mentioned this in T13278: Improve repository Staging Areas.Apr 6 2019, 5:14 PM
epriestley moved this task from Backlog to Staging Areas on the Diffusion board.Apr 15 2019, 3:39 PM
Herald added a subscriber: amckinley. · View Herald TranscriptApr 15 2019, 3:39 PM
Phabricator · Built by Phacility