Page MenuHomePhabricator

Add a formal "Draft" / "Not Yet Ready for Review" state to Differential
Open, NormalPublic

Description

This is a use case I just ran into, and I'm looking for feedback, but maybe it could lead to a feature request.

Sometimes, I will be working on a feature and I reach a point where I'd like to review where I'm at before continuing further. I can't finish the feature, because I need to verify that what I've done so far is OK with the rest of the team.

Just now, I submitted the revision using arc diff --plan-changes, which is ok -- but it doesn't show up in the reviewer's "Blocking Others" list; which it is, because I need a review before I can move forward with coding.

The alternative is to submit it for review without pending changes, but this runs the risk of accepting and landing an incomplete feature if the reviewer is not especially careful about reading through my comments.

I'm wondering if it would be worth having a sort of "Plan changes pending review" state? Or if there's a better workflow for this type of scenario.

Revisions and Commits

rARC Arcanist
D18743
D18740
D18739
D18738
D8400
rP Phabricator
D19810
D18819
D18802
D18801
D18771
D18771
D18765
D18748
D18745
D18737
D18731
D18730
D18721
D18714
D18713
D18628
D18627
D18626
D18625
D18493
D18493
D18490
D18490
D18419
D18418
D18417
D18416
D18415
D18414
D18413
D18412
D18410
D18408
D18400
D18399
D18399
D18398
D18397
D18396
D18395
D18394
D18393
D18392
D18343
D18341
D18340
D18339

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I've landed the two scary migration changes (D18418, D18419) and deployed them here without any obvious issues arising. The actual effects here are:

  • Phabricator now stores revision status string constants (like "accepted") everywhere, and no longer stores older numeric constants (like "2").
  • The differential.revision.search Conduit API method now allows querying by status.
  • The differential.revision.search Conduit API method now returns revision status information.
  • Phabricator no longer depends on the ArcanistDifferentialRevisionStatus class, defined in Arcanist.
  • The differential.find Conduit API method, which has had no upstream callers for about five years, has been removed.

This roughly corresponds to unearthing most of the technical debt here and leaving us free to pursue new server-defined statuses.

alexmv added a subscriber: alexmv.Sep 14 2017, 11:05 PM

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.
hskiba added a subscriber: hskiba.Oct 22 2017, 2:46 AM
OCram added a subscriber: OCram.Oct 23 2017, 5:35 AM

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.

edibiase removed a subscriber: edibiase.Oct 31 2017, 6:55 PM

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.

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.

Pawka added a subscriber: Pawka.Jun 26 2018, 1:44 PM

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

kylec added a subscriber: kylec.Jun 27 2019, 1:58 AM

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

maroux added a subscriber: maroux.Sep 5 2019, 9:56 PM