Page MenuHomePhabricator

Add support for "Plan Changes" as a condition for Differential Revision in Herald
Closed, DuplicatePublic

Description

Root problem: We don't want to run build jobs for revisions with planned changes.

Event Timeline

You've personally observed a build trigger as the result of a user choosing "Plan Changes"?

I don't know about that.

What I'm talking about is creating a revision with plan changes flag upfront from arcanist does trigger a build. I'm assuming updating one with that flag with also trigger a build. We don't want that.

I understand why this may fall under T2543 but this other task appears to be of much bigger scope and take a very long time to be done, while this seems like a minor enhancement. Plan changes is just an attribute of the revision like summary, reviewers, etc... which you can use as conditions in herald, so why not just add it to herald.

Alternatively can we get plan changes as an arg passed to the build system by harbor master?

The build system should be able to call Phabricator via Conduit and ask for revision status today if you want a short-term fix.

Note that differential.revision.search doesn't return a "status" value yet, because I want to resolve T2543. You can get status via the frozen differential.query method. The constants are defined in ArcanistDifferentialRevisionStatus, and "Changes Planned" is 5.

Got it thanks. And would a build be triggered by plan changes flag through the web form? Or only with another arc diff call?

Here's the intended behavior from the source:

protected function shouldApplyHeraldRules(
  PhabricatorLiskDAO $object,
  array $xactions) {

  if ($this->getIsNewObject()) {
    return true;
  }

  foreach ($xactions as $xaction) {
    switch ($xaction->getTransactionType()) {
      case DifferentialTransaction::TYPE_UPDATE:
        if (!$this->getIsCloseByCommit()) {
          return true;
        }
        break;
      case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE:
        // When users commandeer revisions, we may need to trigger
        // signatures or author-based rules.
        return true;
      case DifferentialTransaction::TYPE_ACTION:
        switch ($xaction->getNewValue()) {
          case DifferentialAction::ACTION_CLAIM:
            // When users commandeer revisions, we may need to trigger
            // signatures or author-based rules.
            return true;
        }
        break;
    }
  }

  return parent::shouldApplyHeraldRules($object, $xactions);
}

That is:

  • Run Herald when a revision is created.
  • Run Herald when new changes are submitted, unless those changes were generated automatically in response to discovering a commit for the revision.
  • Run Herald when a revision is commandeered the new way.
  • Run Herald when a revision is commandeered the old way (legacy support code obsoleted by T11114).
  • Otherwise, do not run Herald (the behavior in parent is just return false;).

Basically, we don't want to run builds when users comment, add reviewers, accept revisions, change subscribers, etc.