Implement build generations in Harbormaster
ClosedPublic
Actions

Authored by hach-que on Aug 21 2014, 12:40 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T5932: Archive previous builds (that have been restarted) instead of deleting targets, artifacts and logs
T5936: Support terminating builds, terminating remote processes and build steps

Commits

Restricted Diffusion Commit
rPefadfbbc97f9: Implement build generations in Harbormaster

Summary

Ref T5932. Ref T5936. This implements build generations in Harbormaster, which provides the infrastructure required to both show users the previous states of restarted builds and to allow users to forcefully abort builds (and their targets).

You can view previous generations of a build by adding ?g=<n> to the URI, but this isn't exposed in the UI anywhere yet.

Test Plan

Ran a build plan with a Sleep step in it. Reconfigured it for various sleep times and viewed previous generations of the build after restarting it.

Diff Detail

Repository

rP Phabricator

Branch

build-gen

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/harbormaster/engine/HarbormasterBuildEngine.php:134	XHP16	TODO Comment

Unit

No Test Coverage

Build Status

Buildable 2309
Build 2313: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que updated this revision to Diff 24846.Aug 21 2014, 12:40 PM

hach-que retitled this revision from to Implement build generations in Harbormaster.

hach-que updated this object.

hach-que edited the test plan for this revision. (Show Details)

hach-que added a reviewer: epriestley.

hach-que added tasks: T5936: Support terminating builds, terminating remote processes and build steps, T5932: Archive previous builds (that have been restarted) instead of deleting targets, artifacts and logs.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptAug 21 2014, 12:40 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B2308: Diff 24846.Aug 21 2014, 12:41 PM

Move the restarts above "Status" so the padding doesn't look weird

epriestley added inline comments.Aug 21 2014, 12:45 PM

resources/sql/autopatches/20140821.harbormasterbuildgen.1.sql
3	These should probably be INT UNSIGNED unless you have scary plans for negative generations
src/applications/harbormaster/engine/HarbormasterBuildEngine.php
132	This gets saved as soon as we return. Can we omit this?
src/applications/harbormaster/query/HarbormasterBuildQuery.php
115–116	Can we use `withBuildGenerations()` here instead of `foreach` below?

Harbormaster completed remote builds in B2309: Diff 24847.Aug 21 2014, 12:45 PM

hach-que added inline comments.Aug 21 2014, 12:53 PM

src/applications/harbormaster/query/HarbormasterBuildQuery.php
115–116	No, because the generation number is particular to each build PHID, so there's no way to say "with build PHIDs and associated build generations". For example, if I'm fetching two builds, one at generation 1 and another at generation 2, I don't want to retrieve generation 1 targets on the generation 2 build, thus the foreach is required to do filtering. I can probably throw `array_unique` in there, but it's questionable how many results that will reduce it by.

Changes requested in code review

epriestley requested changes to this revision.Aug 21 2014, 12:54 PM

epriestley edited edge metadata.

epriestley added inline comments.

src/applications/harbormaster/query/HarbormasterBuildQuery.php
115–116	Oh, right. We could do some kind of magic junk here where pass pairs in, but since restarts are hypothetically rare anyway this seems reasonable as-is.