Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4885: Harbormaster build restarts publish empty transactions
T4886: Not apparent who restarted things in Harbormaster or when - Commits
- Restricted Diffusion Commit
rPf2c0e94ea821: Show command transactions in Harbormaster builds
Restart builds and buildables, look at timeline.
Diff Detail
- Repository
- rP Phabricator
- Branch
- harbormaster_txs
- Lint
Lint Passed Severity Location Code Message Advice src/applications/harbormaster/editor/HarbormasterBuildableTransactionEditor.php:66 XHP16 TODO Comment Advice src/applications/harbormaster/editor/HarbormasterBuildableTransactionEditor.php:95 XHP16 TODO Comment - Unit
No Test Coverage - Build Status
Buildable 445 Build 445: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
I may have gone too far here by moving the command creation to the editor.
| src/applications/harbormaster/editor/HarbormasterBuildableTransactionEditor.php | ||
|---|---|---|
| 66 | I had to explicitly attach the build to the buildable in the right place... What's a better alternative? | |
| src/applications/transactions/storage/PhabricatorApplicationTransaction.php | ||
| 650–655 | This makes the now-empty transactions render something. | |
| 669 | I'll fix that. | |
Oh! Sorry, I forgot exactly how this was put together. 90% of this looks good, but I think we should adjust it slightly:
- When the actual command is issued to a build, record a transaction on the Build, not the Buildable. Issuing the actual command from within the Editor is fine and desirable, just move that bit to the BuildableEditor instead of the BuildEditor.
- Record a transaction on the Buildable only for the Buildable-level commands -- and this one is probably best as a pure no-op. So the Buildable-level command looks like:
$editor->applyTransactions($buildable, ...); foreach ($buildable->getBuilds() as $build) { // ... $build->applyTransctions($build, ...); }
- So we'll end up with two TYPE_COMMAND transactions, one on the Buildable (meaning "the entire buildable was restarted") and one on the Build (meaning "this build was restarted").
| src/applications/harbormaster/controller/HarbormasterBuildActionController.php | ||
|---|---|---|
| 58–70 | This would swap to BuildTransactionEditor. | |
| 69–70 | This should be a little simpler after that change. | |
| src/applications/harbormaster/controller/HarbormasterBuildableActionController.php | ||
| 64–69 | This is perfect, but then do a loop with BuildTransactionEditors. | |
| src/applications/harbormaster/controller/HarbormasterBuildableViewController.php | ||
| 56 | (Minor whitespace.) | |
| src/applications/harbormaster/editor/HarbormasterBuildableTransactionEditor.php | ||
| 51 | I'd just no-op this and lift the loop out. | |
| src/applications/harbormaster/storage/HarbormasterBuildableTransaction.php | ||
| 4 | This all looks perfect. | |
| 109 | array_merge() has perf issues when used like this, prefer: $phids = parent::getRequiredHandlePHIDs();
if ($this->getBuildPHID()) {
$phids[] = ...;
}
return $phids; | |
But then won't we see each action n times?
<< a restarted this buildable << a restarted Build 1: some build << a restarted Build 2: some other build
The Buildable will only show the top-level transaction ("alincoln restarted this entire buildable."). The Builds would each show one transaction ("alincoln restarted this specific build."). Feed would have a pile of mess but we should just keep these out of feed/mail/notifications since they're not very important.
| resources/sql/patches/20140514.harbormasterbuildabletransaction.sql | ||
|---|---|---|
| 2 | Oh -- one minor thing that I'll fix in the pull: To get this to automatically apply now, drop it in autopatches/ instead of patches/. The fix is just to move the file. Old stuff in patches/ needed additional boilerplate "actually apply this patch" code which was really redundant and silly so we ditched it a while ago. There might be some docs I need to update somewhere. | |