Render build status on revisions and commits
ClosedPublic
Actions

Authored by hach-que on Nov 9 2013, 6:08 AM.

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.