Page MenuHomePhabricator

Subsume 'harbormaster.querybuilds' with a modern search API method
ClosedPublic

Authored by yelirekim on Jul 31 2016, 8:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 21, 10:19 PM
Unknown Object (File)
Mon, Nov 21, 10:18 PM
Unknown Object (File)
Mon, Nov 21, 10:17 PM
Unknown Object (File)
Mon, Nov 21, 10:17 PM
Unknown Object (File)
Mon, Nov 21, 10:17 PM
Unknown Object (File)
Fri, Nov 18, 9:56 AM
Unknown Object (File)
Thu, Nov 17, 10:32 PM
Unknown Object (File)
Sun, Nov 13, 5:52 PM
Subscribers

Details

Summary

We deprecate the existing API method used to access build information from the API, but preserve its response structure after calling through to the new method. I've cordoned off the fields I needed to define in order to meet the output structure by putting those fields in a search attachment.

Test Plan

Used the API console and looked at the list view controller for builds.

Old output structure:

{
  "data": [
    {
      "id": "16823",
      "phid": "PHID-HMBD-xghrwfz6luoye5rgc2hq",
      "uri": "https://secure.phabricator.com/harbormaster/build/16823/",
      "name": "Run Core Tests",
      "buildablePHID": "PHID-HMBB-s6ykzm2jzxz4ymduztq3",
      "buildPlanPHID": "PHID-HMCP-pcfxcgyoif67l3buc4zt",
      "buildStatus": "passed",
      "buildStatusName": "Passed"
    }
  ],
  "cursor": {
    "limit": 100,
    "after": "16823",
    "before": null
  }
}

New output structure:

{
  "data": [
    {
      "id": 1,
      "type": "HMBD",
      "phid": "PHID-HMBD-qpgcmv67tzaauzayzit5",
      "uri": "http://ec2-54-165-244-168.compute-1.amazonaws.com/harbormaster/build/1/",
      "name": "arc lint + arc unit",
      "buildStatusName": "Passed",
      "buildablePHID": "PHID-HMBB-qdefith5uakkepqpjr2g",
      "buildPlanPHID": "PHID-HMCP-zswbhazb7ipmaf4plygg",
      "buildStatus": "passed",
      "initiatorPHID": "PHID-USER-rihx4366f3aczsvc2wtb",
      "dateCreated": 1450295643,
      "dateModified": 1450295644,
      "policy": {
        "view": "users",
        "edit": "users"
      }
    }
  ],
  "maps": {},
  "query": {
    "queryKey": null
  },
  "cursor": {
    "limit": 100,
    "after": null,
    "before": null,
    "order": null
  }
}

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

yelirekim retitled this revision from to Subsume 'harbormaster.querybuilds' with a modern search API method.
yelirekim updated this object.
yelirekim edited the test plan for this revision. (Show Details)
src/applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php
11 ↗(On Diff #39332)

Whoops, didn't mean to do this.

epriestley added a reviewer: epriestley.
epriestley added inline comments.
src/applications/harbormaster/storage/build/HarbormasterBuild.php
421

Maybe 'phid'.

430

Maybe model this on ManiphestTask, e.g.:

'status' => array(
  'value' => 'passed',
  'name' => "Passed",
)

Then we can add more stuff later with a minimal compatibility break.

437

This needs to be id(new ...), not just (new ...), to compile/run on older PHP.

This revision is now accepted and ready to land.Jul 31 2016, 8:23 PM
src/applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php
11 ↗(On Diff #39332)

Either way is probably fine, although maybe just keep it if it was accidental rather than intentional to increase clarity.

yelirekim marked 4 inline comments as done.
yelirekim edited edge metadata.
  • linty things
  • correctly set field specification to phid
src/applications/harbormaster/storage/build/HarbormasterBuild.php
430

I always like to keep API responses "normalized" and stray away from unstructured (eg, a name or a url) data as much as possible. Unless, of course, there is an underlying use case :)

Yeah, I tend to agree.

There are soft use cases for, Maniphest and Differential, since some arc commands (arc list, arc branch) need to show status names and colors. We could implement maniphest.status.search and do a second call to lookup status information, but that's slower and creates additional API surface area without necessarily providing a great deal of value, especially if this is the only real use case. (On the other hand, reasonable systems may need to query all the statuses, which would mean we'd need maniphest.status.search anyway.)

We could also provide a "status" attachment which supplemented status information. This is somewhat inefficient because of denormalization (you may get 100 copies of "open" status information) and somewhat complex, but at least a plausible tool we might use in this or similar situations.

To me, the best argument for returning dictionaries is that we know there's reasonable information we could return if we decided that the API should heavily favor convenience over normalization, and we have more flexibility to actually return it later in a clean way should use cases arise if we return a dictionary now. If we don't, we probably need to add top-level keys and end up with buildStatus, buildStatusName, buildStatusColor, buildStatusIcon. Hardly the end of the world, but not quite as nice.

I don't currently have any plans to implement arc builds, though.

I do have plans to implement arc builds...

Return a dictionary for the build status over conduit.

This revision was automatically updated to reflect the committed changes.
yelirekim marked an inline comment as done and an inline comment as not done.
src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php
59–62

I missed these, but the [] shorthand isn't valid in older PHP; see T11409. We should probably have lint for this.

Gah, I knew I wasn't supposed to do that, short array syntax is wired into my brain unfortunately. I'll fix them up.

In PHI261, an install reported arc land coloring build results incorrectly in the display warning. This is because we accidentally changed the behavior of buildStatus for harbormaster.querybuilds here: previously it was a constant, and now it's an array. Since this is a minor cosmetic issue and this shipped a long time ago, I plan to just upgrade arc and not restore the original behavior.

Also, PHP allowing switch ($array_value) { ... } is dumb but I can't come up with any reasonable way to prevent it unless we want to rewrite every switch into switch(assert_scalar($value)) { ... }, which I don't particularly. I'll make a note on T2312 about strictness, since we could patch PHP to disallow this, as I'd some day like to patch it to disallow 1 == "string".