Page MenuHomePhabricator

Render build status on revisions and commits
ClosedPublic

Authored by hach-que on Nov 9 2013, 6:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 8:36 AM
Unknown Object (File)
Tue, Mar 26, 8:31 AM
Unknown Object (File)
Tue, Mar 26, 8:31 AM
Unknown Object (File)
Fri, Mar 22, 2:36 AM
Unknown Object (File)
Fri, Mar 22, 2:36 AM
Unknown Object (File)
Fri, Mar 22, 1:54 AM
Unknown Object (File)
Fri, Mar 22, 1:17 AM
Unknown Object (File)
Sat, Mar 16, 7:52 PM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T1049: Implement Harbormaster
Commits
Restricted Diffusion Commit
rP79ef667dfdc8: Render build status on revisions and commits
Summary

This uses an event listener to render the status of builds on their buildables. The revision and commit view now renders out the status of each of the builds.

Currently the revision controller has the results for the latest diff rendered out. We might want to show the status of previous diffs in the future, but for now I think the latest diff should do fine.

There's also a number of bug fixes in this diff, including a particularly nasty one where builds would have a build plan PHID generated for them, which resulted in handle lookups always returning invalid objects.

Test Plan

Ran builds against diffs and commits, saw them appear on the revision and commit view controllers.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

What's the reasoning for removing needBuildPlans()? Everything else here looks good, but I want to make sure I understand that before landing this.

src/applications/harbormaster/event/HarbormasterUIEventListener.php
28โ€“35

At some point, maybe we should add HarbormasterBuildableInterface, although I'm not sure it's practical to imagine that third parties could really extend it with new buildables -- at a minimum, the objects would need some kind of "createThisBuildableFromScratch()" method, which is probably hard. Maybe this is more realistic than I think, though. This is easy to circle back to later, though.

47โ€“50

Isn't this a reasonable case where we don't need to load the BuildPlans? I don't think it's a huge deal to always load them, but they make handles and integrations like this way heavier. If every object loaded related objects that it usually needs, rather than just objects it absolutely needs, I think handle queries would be out of control huge.

51

If there are no builds, maybe we should just return? This will render an empty "Build Status:", I think?

src/applications/harbormaster/storage/build/HarbormasterBuild.php
68

Ugh, thanks for catching this. Sorry about that.

The reason for removing needBuildPlans is that rendering the build name requires the build plan name. You can't access the build plan name if the build plan isn't attached, so calls to getName will fail.