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.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T1049: Implement Harbormaster
- Commits
- Restricted Diffusion Commit
rPe4569e7e7e2e: Implement interface for adding, editing and deleting build steps on plans.
See screenshots:
{F78529}
{F78532}
{F78528}
{F78531}
{F78527}
{F78530}
Diff Detail
Diff Detail
- Branch
- harbormaster3
- Lint
Lint Passed Severity Location Code Message Advice src/applications/harbormaster/controller/HarbormasterStepAddController.php:33 XHP16 TODO Comment Advice src/applications/harbormaster/controller/HarbormasterStepEditController.php:45 XHP16 TODO Comment Advice src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php:71 XHP16 TODO Comment - Unit
No Test Coverage
Event Timeline
Comment Actions
Updated code to validate and list available step implementations by using PhutilSymbolLoader.
Comment Actions
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 | 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 | ||
| 71–78 | (From before, just load and attach it unconditionally. I think this creates better object behavior and the previous policy was the right one.) | |