Page MenuHomePhabricator
Differential D7499

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

Authored by hach-que on Nov 5 2013, 5:48 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 8, 9:13 PM
Unknown Object (File)
Sat, Dec 7, 4:27 PM
Unknown Object (File)
Fri, Dec 6, 4:18 AM
Unknown Object (File)
Wed, Dec 4, 10:02 AM
Unknown Object (File)
Wed, Dec 4, 5:43 AM
Unknown Object (File)
Nov 27 2024, 10:55 AM
Unknown Object (File)
Nov 27 2024, 5:59 AM
Unknown Object (File)
Nov 23 2024, 2:28 PM
View All Files
Subscribers
aran
chad
epriestley
Korvin

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

hach-que updated this revision to Unknown Object (????).Nov 5 2013, 5:55 AM

Remove redundant parameter.

epriestley requested changes to this revision.Nov 5 2013, 6:50 PM

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.

hach-que updated this revision to Unknown Object (????).Nov 5 2013, 9:28 PM

Made changes requested in code review.

hach-que updated this revision to Unknown Object (????).Nov 5 2013, 9:30 PM

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.

epriestley closed this revision.Nov 5 2013, 9:35 PM

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

Revision Contents
Changeset List

PathSize
resources/
sql/
patches/
11 lines
src/
9 lines
applications/
harbormaster/
query/
38 lines
step/
48 lines
24 lines
storage/
configuration/
51 lines
worker/
22 lines
infrastructure/
storage/
patch/
4 lines

Diff 16925

View Options

resources/sql/patches/20131105.buildstep.sql

Loading...
View Options

src/__phutil_library_map__.php

Loading...
View Options

src/applications/harbormaster/query/HarbormasterBuildStepQuery.php

Loading...
View Options

src/applications/harbormaster/step/BuildStepImplementation.php

Loading...
View Options

src/applications/harbormaster/step/SleepBuildStepImplementation.php

Loading...
View Options

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

Loading...
View Options

src/applications/harbormaster/worker/HarbormasterBuildWorker.php

Loading...
View Options

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

Loading...
Phabricator · Built by Phacility