Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T1049: Implement Harbormaster

Commits

Restricted Diffusion Commit
rPc514d34b9456: Add build step implementation infrastructure and sleep build step.

Summary

Depends on D7498.

This implements support for a "build step implementation". Build steps have an associated class name (which makes the class in PHP) and a details field, which is serialized JSON (same as PhabricatorRepository).

This also implements a SleepBuildStepImplementation which just pauses the build for a specified period of seconds.

Test Plan

Inserted a build step with insert into harbormaster_buildstep (phid, buildPlanPHID, className, details, dateCreated, dateModified) values ('', 'PHID-HMCP-zkh5w6czfbfpk2gxwdeo', 'SleepBuildStepImplementation', '{"seconds":5}', NOW(), NOW()); (adjusting the build plan PHID as appropriate).

Started the daemon and applied the build plan to a buildable, and saw the daemon take a 5 second delay after creating SleepBuildStepImplementation.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

Remove redundant parameter.

Minor inlines, didn't catch anything substantive. The rest of this looks great to me, but will probably need some adjustments on top of D7498 if we massage the daemon model there a bit. The actual daemon code here looks fine, but I think we should consider putting it in a Worker.

resources/sql/patches/20131105.buildstep.sql
9	I imagine we'll want some step/sequence fields here eventually, but I'm not sure what they'll look like quite yet. This object otherwise looks like I'd expect it to look.
10	We should probably also have: KEY `key_plan` (buildPlanPHID)
src/applications/harbormaster/step/BuildStepImplementation.php
35	Could this be called implicitly by getSettings()? Or the logic performed implicitly? Seems like it might be simplifiable out of the public API. Not a real issue.
src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
49–51	`is_subclass_of()` is a cheap way to nearly do this, although it won't test for abstract subclasses. `PhutilSymbolLoader` can also do it, although it's like 10 lines long.
52	Prefer `newv()` for consistency.
53–55	I believe this is impossible to reach. I don't know any way to make `new` return `null` and continue execution.
71	`BuildStepQuery` should load and attach `BuildPlans` unconditionally (in `willFilterPage(array $page)`) if the build plan is required for policy checks.
75	For consistency, call into the build plan to figure this out, too.
79	This should probably return some string like "A build step has the same policies as its build plan."

hach-que added inline comments.Nov 5 2013, 9:14 PM

src/applications/harbormaster/step/BuildStepImplementation.php
35	The build engine calls loadSettings() against the implementation, and the implementation then calls getSettings(). At the point where the implementation is executing, it only has the Build object as the parameter, not the BuildStep that owns it, so you need to load in the data ahead of time. It would also be a bit redundant to pass in the BuildStep to the `execute` method since the only valid use for accessing that object is to get settings.
src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
49–51	I actually did the PhutilSymbolLoader stuff in D7500, so I'll backport it to this diff.

Made changes requested in code review.

Redoing diff against master...?

epriestley accepted this revision.Nov 5 2013, 9:34 PM

epriestley added inline comments.

src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
76	This should be "its", I think.

Closed by commit rPc514d34b9456 (authored by @hach-que, committed by @epriestley).

Add build step implementation infrastructure and sleep build step.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 16925

resources/sql/patches/20131105.buildstep.sql

src/__phutil_library_map__.php

src/applications/harbormaster/query/HarbormasterBuildStepQuery.php

src/applications/harbormaster/step/BuildStepImplementation.php

src/applications/harbormaster/step/SleepBuildStepImplementation.php

src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php

src/applications/harbormaster/worker/HarbormasterBuildWorker.php

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

Add build step implementation infrastructure and sleep build step.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 16925

resources/sql/patches/20131105.buildstep.sql

src/__phutil_library_map__.php

src/applications/harbormaster/query/HarbormasterBuildStepQuery.php

src/applications/harbormaster/step/BuildStepImplementation.php

src/applications/harbormaster/step/SleepBuildStepImplementation.php

src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php

src/applications/harbormaster/worker/HarbormasterBuildWorker.php

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

Add build step implementation infrastructure and sleep build step.
ClosedPublic
Actions

Revision Contents
Changeset List