An unusual case here is Herald rules which take the "Send me an email" action, and act "only the first time". Their current behavior will be to activate when the draft is created, have their mail killed later in the pipeline when the Editor declines to generate any, and then never fire again.

I think I'm maybe going to add a special case to Herald to deal with this, an exception like "RetryThisActionLaterException". If an action raises this exception, it will act like the entire rule failed.

This will still create a little bit of odd behavior if you have a rule which takes multiple actions, like "send me an email" and "add project tags" or something -- the tag part could act on a draft, but won't, and the whole rule will act after the revision publishes.

This also means we need to fire Herald more often -- essentially on every update, not just every major change (see T10109). If we don't, these "email me" rules won't activate when a revision leaves the "draft" state.

But I think we can do that, and just make the "Run Build Plan" rules also throw a "RetryThisActionLaterException" if the revision wasn't just updated. Kind of a lot of rules behind the scenes but I think the actual behavior will mostly be intuitive, and the edge cases (a rule which both sends email and runs a build plan, e.g.) can be resolved by splitting rules apart if they do arise. This should also let us resolve T10109.

A very rough version of this is now in master, but only active if phabricator.show-prototypes is on. These are some of the (substantial) remaining issues I'm aware of, and there are probably others:

• Trivially, email doesn't really respect drafts yet, so the whole thing is semi-pointless, but likely will soon.
• We currently (well, after the change above) mute the initial email entirely if a revision is a draft, but I think I want to continue sending it, just only to the author. Maybe? The email would be rephrased to say "This is a draft, and waiting on builds X, Y, Z."
• When a revision gets stuck in a draft state because of a failed build, I suspect the notifications and cues are currently insufficient. For example: the /differential/ UI won't distinguish between "building draft", "static draft", and "failed draft", and I think the email notifications will be limited and/or misleading.
• The "Send me an email only the first time" interaction described above won't work. I plan to implement a ThisCondition/ActionDoesNotWorkInThisState exception, per above.
• The client has no awareness of this state yet, so you can't arc diff into it explicitly (e.g., with --hold) and stuff like arc branch won't color revisions in this state properly (in fact, it may not even know that this state exists).
• We don't track if a revision has returned to a draft state. I plan to discard the "draftiness" of a revision permanently the first time it leaves the draft state so reviewers are immediately notified of later updates, but there's currently no tracking of this state.

This is technically accurate but real awkward for humans:

After all that stuff:

• Herald should now interact properly with drafts.
• Email (and other build-related cues) are still a bit weird and need some more refinement.
• With a sufficiently up-to-date server and arc on the experimental branch, arc diff --draft should work (once everything here lands). See T11911 for other experimental changes and a rundown of new arc diff --flag behaviors. --preview, --only, --plan-changes, and --draft have changed. These changes ONLY affect the experimental branch.
• See T13010 for a more concise summary of the status of this feature.

From T10109, we shouldn't make revisions "Buildable" to Herald if the update is an automatic update in response to a commit being discovered.

The harbormaster builds that we have running on revision creation can sometimes be rather slow; they're builds in our primary CI system, but with less of a priority than jobs in the submit queue. This will result in 10-30 minute delays for every revision in our monorepo. I understand the preference for only adding configuration settings when absolutely necessary, but since this may result in a significant time sink for any installs with expensive builds, is it worth keeping something around like differential.initial-state?

Maybe. Another possibility is to mark builds as "nonblocking" in some sense. There are some other open requests for defining "advisory" or "noncritical" builds where build failure doesn't cause the Buildable to report a failed result overall. If we end up adding this state to builds anyway, it probably makes sense to include a "Does Not Block Review" flag to the "Does Not Fail Buildable" / "Does Not Block Dependent Builds" / "Failure Is Totally Meaningless" / Whatever flags.

If this is a pain in the short term, you can tweak DifferentialRevision::initializeNewRevision() as a workaround until we figure out the plan in the longer term.

(Another possibility is to add behavior to arc to control this.)

@epriestley Are there any more concrete plans for this "nonblocking" builds? That is something we would like to utilise for one of our projects. Actually quite important as waiting for builds there is slowing us down quite a bit now.

No.

GHC recently upgraded its Phabricator installation and has been rather unpleasantly affected by this feature. We have multi-hour CI jobs, which often queue to a day or so of builds. Making contributors wait this long before receiving feedback is turning out to be rather problematic.

Am I correct in thinking that this patch should be sufficient to revert to the previous behavior?

diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
index 6813c12..f1b789c 100644
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -72,11 +72,7 @@ final class DifferentialRevision extends DifferentialDAO
     $view_policy = $app->getPolicy(
       DifferentialDefaultViewCapability::CAPABILITY);
 
-    if (PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) {
-      $initial_state = DifferentialRevisionStatus::DRAFT;
-    } else {
-      $initial_state = DifferentialRevisionStatus::NEEDS_REVIEW;
-    }
+    $initial_state = DifferentialRevisionStatus::NEEDS_REVIEW;
 
     return id(new DifferentialRevision())
       ->setViewPolicy($view_policy)

@epriestley, can you confirm that the above patch should disable the draft state? I have applied it to GHC's Phabricator deployment yet we are still seeing Differentials opened in draft state.

@bgamari Did you ever find a solution?

You will soon be able to give builds different "Hold Drafts" behaviors. See T13258.

• Always: Current behavior. Revisions wait for the build to finish before promoting, and are returned to the author if the build fails.
• If Building: For quick but untrustworthy builds. Revisions wait for the build to finish before promoting, but still promote and are sent for review even if the build fails.
• Never: For exceptionally long-running builds. Revisions ignore the build status when making a promotion decision.

The behavior of each build may be configured independently, so you may use a mixture of different rules if you have a mixture of quick/slow or reliable/unreliable builds.

This will likely reach stable this week, in 2019 Week 9.

After that, possibly as soon as 2019 Week 10, I plan to remove the "prototype" constraints around this feature and make it the only behavior in all cases, since I think this is the only meaningful blocker/issue with the "Draft" state.

Any update on when this is going to reach stable? We're ~16 weeks past the earlier estimate...

Is there a way to disable this option via a config in the server or a CLI parameter?

Sometimes I don't want to create a draft but to send a diff directly for review. For example, for simple diffs, we send them directly for review, even before the diff finishes building. Reviewers will accept these simple changes because if the diff fails to build later, then the arc land would forbid the diff from landing. But it seems now this is not possible and the reviewers need to wait until the diff finishes building before accepting.

We just found a real issue with this, different from what I mentioned above: We had an issue with our CI machine, and to fix it we needed to make a diff. But as the CI machine is not working, the diff won't build, which means we can't accept the diff that will solve the issue.

@epriestley Even if "draft" is the default, is it possible to add a CLI argument to choose "needs_review"?

I took a look at this and I think the option was completely removed: https://secure.phabricator.com/rPdc35ce79e484373eaa3964e8769e9da3c7091e9d

You can open the revision in the web UI and "Request Review". You can use arc diff --browse ... to open the web UI automatically.

You can technically "Request Review" from the CLI by using the API (arc call-conduit differential.revision.edit) but there's no convenient UI on top of it (like a --skip-draft-mode flag). I don't currently plan to add one outside of T11934.

Thanks! I didn't know it could be done from the web UI.