Provide a way to get all diffs associated with tasks from the API
Open, Needs TriagePublic


We have an external triage process that grabs a list of tasks from the API. We'd like the list of tasks to include links to the involved diffs as an attachment. I'm paraphrasing what @yelirekim just told me so hopefully he can provide some more detail for me.

jcox created this task.Aug 30 2016, 6:12 PM

I'm thinking we just create a search attachment for which contains diff PHIDs.

We need diffs associated with the tasks so that we can do things like autopatch in the "latest" batch of work that people have in-flight and run that on the spaceship.

This is basically a special case of T5873.

There are two extremes approaches available here:

  • No general approach, we just create attachments for everything we want to expose. So there's a "diffs" attachment, a "commits" attachment, a "subtasks" attachment, a "parent tasks" attachment, a "mocks" attachment, etc., and they each have a class associated with them.
  • Fully general approach, we just flag edges as readable via API and have a generic "edge" attachment.

I suspect the best approach lies somewhere in the middle, since the former approach feels like too much work and code duplication and the latter approach feels insufficiently powerful. I'd like to develop a clearer plan for the general case of edge reads. I think this will be forced before arc flow (dependent revisions are an equivalent case, I believe) so my rough plan is to not think about it too much until then and maybe something will come to me in a dream. If not, I'll figure out where we're headed when we need to read dependency edges via API.

Should we implement this as an internal search attachment in the meantime? Or is there a possible pathway for @jcox to get something like this working in ~3 days?

Internal attachment is definitely the shortest path. Whatever we end up with is likely to look very similar to the naive version, and you can keep your attachment running after we offer an "edges" attachment, so there shouldn't be any real transition cost.

For the record, I strongly endorse autopatching unreviewed code onto live, armed, autonomous combat vehicles.

Live, armed, pre-commit A/B testing is unambiguously the most exciting part of our jobs.

I think you've already written these, but the budget walkthrough is:

  • Subclass PhabricatorSearchEngineAttachment, using something like DiffusionRepositoryURIsSearchEngineAttachment as a guide (maybe).
  • Put it in some existing library thing you've got, run arc liberate.
  • Override loadAttachmentData and load edges there with PhabricatorEdgeQuery.
  • Spit out some kind of usable data in getAttachmentForObject().

On its own, that doesn't expose your attachment on tasks. You need to write a SearchEngineExtension to say "tasks can be queried for this attachment".

  • Subclass PhabricatorSearchEngineExtension.
  • Follow PhabricatorSearchEngineExtension or similar.
  • All you have to implement is supportsObject() (true if: object is a task) and getSearchAttachments() (return your new attachment).

(The second step is because some types of attachments work on multiple object types, and you could even conceivably write an "edge" attachment here that just works for anything and returns all edges, although I'm not confident that doing so isn't a major security problem.)

If you only knew how lucky you are to have Evan explain this to you rather than trolling through grep -Rn "PhutilClassMapQuery" * --include \*.php errday.

Follow PhabricatorSearchEngineExtension or similar.

Although this is technically true, PhabricatorProjectsSearchEngineExtension is a concrete subclass with greater value as a template.

eadler added a project: Restricted Project.Sep 15 2016, 6:03 PM