Implement basic Harbormaster daemon and start builds.
ClosedPublic
Actions

Authored by hach-que on Nov 5 2013, 4:57 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T1049: Implement Harbormaster

Commits

Restricted Diffusion Commit
rPca5400d14bcb: Implement basic Harbormaster daemon and start builds.

Summary

This implements a basic Harbormaster daemon that takes pending builds and builds them (currently just sleeps 15 seconds before moving to passed state). It also implements an interface to apply a build plan to a buildable, so that users can kick off builds for a buildable.

Test Plan

Ran bin/phd debug PhabricatorHarbormasterBuildDaemon and used the interface to start some builds by applying a build plan. Observed them move from 'pending' to 'building' to 'passed'.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

I just realised that this isn't hooked up to the "Manually Execute Plan" link on the plan details page and instead adds it's own "Apply Build Plan" link against the buildable. We can either remove the "Manually Execute Plan" link or "Apply Build Plan" link.

Personally, I don't think executing the plan from the plan details page makes a lot of sense; the actual build appears on the buildable's detail page, not the plan's detail page, so one would have to navigate to the buildable after executing the plan to actually access the build.

Personally, I don't think executing the plan from the plan details page makes a lot of sense; the actual build appears on the buildable's detail page, not the plan's detail page, so one would have to navigate to the buildable after executing the plan to actually access the build.

My reasoning on this is mostly just lazy: it's easy to identify/find/query/lookup a buildable by string ("D123", "rE123") but harder to identify a build plan by string if an install has, like, 1,000 of them, and I don't know if they're important enough to give object names to. "Apply Build Plan" (that is, an action on a Buildable) is likely better in the long run, but we'll need to make it pop some fancier UI sooner once installs get more build plans than fit in a <select /> (although maybe everyone will just have 20-ish build plans for a while and we have a lot of runway here).

The other part is that triggering it from the build plan can create a new buildable. I think this is a good thing -- that rE123 can be represented as a buildable multiple times in Harbormaster in order to debug/test things -- to the point that I felt like making it the default action might be desirable. But this is probably also going to be super confusing to users in some cases. I think it's worth retaining the capability, but could go either way on forcing it on users.

Overall, I'm completely fine with replacing my lazy unintuitive link with one that makes sense but has some theoretical UI issues.

Upshot:

Most of this looks great.
Daemon construction has some substantive comments, let me know if those make sense.
Remove "Execute Manual Build" and let's move forward with "Apply Build Plan".

src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php
51–53	Yeah, this was exactly what I hit.
58–59	Missing `pht()` I think cancelButton is `($uri, $name)`? Does this button work?
60–71	If this looks funny, you can wrap it in `AphrontFormLayoutView` to get more normal CSS.
src/applications/harbormaster/controller/HarbormasterBuildableViewController.php
109–116	I think this should be on the detail view, not the main view? In the long run, I think/hope it will be a rare action.
src/applications/harbormaster/daemon/PhabricatorHarbormasterBuildDaemon.php
6 ↗	(On Diff #16904)	I think we can drive this out of the normal task queue instead, and get all the task queue stuff for free. Particularly: One worker responds to new Buildables and runs build plan rules and Herald rules against them. It produces a bunch of Build objects, and adds a new task to the queue for each Build object. We could even do this in-process for now, although it's probably cleaner to put it in the daemons today. The child worker is handed a Buildable and a Build and executes the Build Plan. You can look at `FeedPublisherWorker` for an example of a very similar daemon to the first daemon. Then we'd add code in Diffusion and Differential like: new Buildable()->...->save(); PhabricatorWorker::scheduleTask(..., $buildable->getID(), ...); For the manual builds, we'd just insert the child task: new Build()->...->save(); PhabricatorWorker::scehduleTask(..., $build->getID(), ...); Then we don't have to deal with polling, we can run the queue faster by launching more taskmasters, all the locking is dealt with, we don't need to run a new daemon, etc. I think this makes everything cleaner. The only thing that gets a little messier is updating the overall build status, but I think that's not too complicated.
src/applications/harbormaster/storage/HarbormasterBuildable.php
15	oh perfect our work here is done
src/applications/harbormaster/storage/build/HarbormasterBuild.php
13–41	These seem reasonable to me. Some other states we might need eventually: Error: we tried to execute the build but hit an unexpected exception or something, or the build plan is garbage, or whatever. Partial Success: the build was fine but the documentation didn't generate or something like that? Something like "non-blocking build steps". This is probably far away. It might be more important at the Buildable level than the Build level, since separating docs out into a separate build plan is probably a better strategy. But if the documentation needs artifacts from this build plan, maybe it ends up in here some of the time. Paused/Aborted: User intervention? Not sure we'll need this.

Made changes requested in code review.

Some tiny inlines, I'll just adjust those in the pull.

src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php
45	This should `->setURI($buildable_uri)` to cover non-JS cases.
65	I think this one is `($uri, $label)` too?
src/applications/harbormaster/worker/HarbormasterBuildWorker.php
24–26	There's a PermanentFailureException you could throw here.
src/applications/harbormaster/worker/HarbormasterRunnerWorker.php
3	Ah, nice catch to get rid of this.