Page MenuHomePhabricator

Implement interface for adding, editing and deleting build steps on plans.
ClosedPublic

Authored by hach-que on Nov 5 2013, 7:19 AM.
Tags
None
Referenced Files
F14029406: D7500.id16911.diff
Fri, Nov 8, 8:39 PM
F14009006: D7500.id.diff
Wed, Oct 30, 8:32 AM
F14007019: D7500.id16927.diff
Mon, Oct 28, 10:04 PM
F14007018: D7500.id16926.diff
Mon, Oct 28, 10:04 PM
F14007017: D7500.id.diff
Mon, Oct 28, 10:04 PM
F14007016: D7500.id16908.diff
Mon, Oct 28, 10:04 PM
F14007015: D7500.id16911.diff
Mon, Oct 28, 10:04 PM
F14006948: D7500.diff
Mon, Oct 28, 9:33 PM

Details

Summary

This implements an interface for adding new build steps, editing existing build steps and deleting build steps from build plans. It uses the settings definitions on the build implementation to work out what fields should be displayed on the edit page.

Test Plan

See screenshots:

{F78529}

{F78532}

{F78528}

{F78531}

{F78527}

{F78530}

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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

Updated code to validate and list available step implementations by using PhutilSymbolLoader.

Minor inlines. Type/validate stuff is a touch funky but fine for now. I think CustomFields are probably the best approach. Probably a good candidate for me to send you an implementation for, since it has a lot of API surface area.

src/applications/harbormaster/controller/HarbormasterPlanViewController.php
91

pht

src/applications/harbormaster/controller/HarbormasterStepAddController.php
24

Vague musing: I didn't add CAN_EDIT to these objects so this is correct, although I wonder if maybe we should at some point. Build plans feel a little less global than, say, Herald global rules.

I think the policies are reasonable for now, but maybe we should keep an eye on the possibility that we might want to lock plans down more eventually. I could imagine plans having credentials and stuff in them, for example.

27

For consistency, just return 404.

38

Not sure that we really need it, but consider adding initializeNewStep($user) for consistency.

42–43

At some point, we should drive this with ApplicationTransactions.

src/applications/harbormaster/controller/HarbormasterStepDeleteController.php
31

Instead of deleting steps, maybe we should just archive/disable them? The major case that I think is interesting is being able to see what the steps used to be if there's some old build, or if a step changes and the build stops working and there was some subtle config thing. I imagine we'd completely hide them from the UI, but the transaction log would still let you get to them.

(I think deleting them is fine for now, but we might want to move away from this eventually.)

src/applications/harbormaster/controller/HarbormasterStepEditController.php
41–62

The easiest way to do this in the long run might be to use CustomField, although it's a lot of work up front. I think this is fine for now. We don't need perfect user-friendliness for "sleep 15 seconds".

src/applications/harbormaster/daemon/PhabricatorHarbormasterBuildDaemon.php
41 ↗(On Diff #16911)

This is maybe a case for STATUS_ERROR or STATUS_BROKEN. The build didn't really fail, it's just impossible to execute.

src/applications/harbormaster/step/BuildStepImplementation.php
7–9

Fine for the moment, but as above I think we should seek to transition these to something like CustomField instead of building another new type system.

14–16

Or:

->loadObjects();
return array_keys($symbols);
src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
75

(From before, just load and attach it unconditionally. I think this creates better object behavior and the previous policy was the right one.)

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

Revert policy change as requested.

Two very minor inlines that I'll just hit in the pull.

src/applications/harbormaster/controller/HarbormasterStepAddController.php
46

As below.

src/applications/harbormaster/controller/HarbormasterStepDeleteController.php
33–40

Missing pht / uri / setURI(), I'll tweak those.