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.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rP98492765d315: Subsume 'harbormaster.querybuilds' with a modern search API method
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
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php | ||
|---|---|---|
| 11 ↗ | (On Diff #39332) | Whoops, didn't mean to do this. |
| 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. | |
| 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. |
| 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 :) | |
Comment Actions
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.
Comment Actions
Gah, I knew I wasn't supposed to do that, short array syntax is wired into my brain unfortunately. I'll fix them up.
Comment Actions
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".