Page MenuHomePhabricator

Can't use `differential.createcomment` with message and action
Closed, ResolvedPublic

Description

This is a recent regression.

Reproduction steps:

Go to https://phabricator.example.com/conduit/method/differential.createcomment/
Fill in `revision_id`, `message`, and `action` fields with valid info.
Click "Call Method".
$ echo '{
  "revision_id": 12345,
  "message": "Testing",
  "action": "accept"
}' | arc call-conduit --conduit-uri https://phabricator.example.com/ --conduit-token <conduit-token> differential.createcomment

Expected result:

Comment is posted and action is set.

Actual result:

ERR-CONDUIT-CORE

Attempting to access attached data on DifferentialRevision (via getActiveDiff()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.

Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly.

Version Information

Event Timeline

Can you use the modern conduit calls?

This method is frozen and will eventually be deprecated. New code should use "differential.revision.edit" instead.

Not easily. :/

Both http://www.dctrwatson.com/2013/01/jenkins-and-phabricator/ and https://github.com/uber/phabricator-jenkins-plugin use differential.createcomment. Would be a pretty large overhaul to use differential.revision.edit instead (which requires the use of PHIDs rather than just differential revision IDs).

Does this work?

diff --git a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php
index 52736d6..943c3e7 100644
--- a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php
+++ b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php
@@ -49,6 +49,7 @@ final class DifferentialCreateCommentConduitAPIMethod
       ->withIDs(array($request->getValue('revision_id')))
       ->needReviewers(true)
       ->needReviewerAuthority(true)
+      ->needActiveDiffs(true)
       ->executeOne();
     if (!$revision) {
       throw new ConduitException('ERR_BAD_REVISION');

That does indeed work! Thank you so much!

epriestley claimed this task.
epriestley added a subscriber: epriestley.

This should be fixed in HEAD of master and stable. Thanks for the report!

Note also that differential.revision.edit accepts IDs and harbormaster.createartifact allows you to create links to build results without needing to gum up the feed with comments. We will eventually remove differential.createcomment so things will need to update sooner or later.

Also, test agents probably shouldn't be accepting revisions at all. But ¯\_(ツ)_/¯

Thanks, everybody! Appreciate it, especially on a Friday night. :-)

Also, test agents probably shouldn't be accepting revisions at all. But ¯\_(ツ)_/¯

That was just differential/revision/diff confusion, we're actually using diff id's, that should be the good strategy right?

The general story on Build/CI is this:

Long ago, we didn't have any formal upstream support for Build/CI. In this era, various users hacked together partial approaches, like @DctrWatson's blog post from 2013. These generally worked by leaving comments with build status and, in some cases, accepting revisions on build pass / rejecting revisions on build failure. They tended to use arc patch to apply revisions in build environments, sometimes triggered based on unusual events (other than "revision updated"), and took other approaches which weren't great but were the best things available at the time.

Since 2013, we built Harbormaster and now have formal methods for triggering builds, handing changes to CI, reporting build status, and attaching "artifacts", including links to external build results. If you use these mechanisms, you get additional integrations. For example: in Differential, reviewers are warned about running or failing builds in the comment area; and arc land warns you about running or failing builds and forces you explicitly "y" through a "Land anyway, even though this is failing?" prompt. You also get consistent triggers, consistent change handoff, etc. These integrations are generally richer than what's possible by leaving comments or taking "accept" actions, and have fewer weird side effects (including simple stuff, like leaving outdated comment clutter in the review timeline).

We recommend installs transition to these modern approaches as time allows. The old approaches won't necessarily stop working (and even if we removed "frozen" API methods, the API is modular and you could just put them back in place), but the upstream has never supported doing build integration through comments / "Accept" and if you report a bug with this to us or ask us to improve something, we'll probably say "switch to the modern stuff which we actually support" before moving forward.

(In this case, our behavior was strictly wrong, completely accidental, and trivial to fix -- but sooner or later we may make some kind of change which isn't very compatible with this older style of integration in a more fundamental way. When we do, a blog post from 2013 suggesting a set of workarounds at the time won't hold back the tides of change.)