Page MenuHomePhabricator

Implement artifact release for Harbormaster
ClosedPublic

Authored by hach-que on Aug 10 2014, 4:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 3:46 AM
Unknown Object (File)
Sat, Nov 23, 3:22 PM
Unknown Object (File)
Sat, Nov 23, 11:13 AM
Unknown Object (File)
Sat, Nov 23, 5:04 AM
Unknown Object (File)
Wed, Nov 20, 12:34 PM
Unknown Object (File)
Sat, Nov 16, 12:48 AM
Unknown Object (File)
Mon, Nov 11, 5:10 PM
Unknown Object (File)
Fri, Nov 8, 4:06 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rPaab0ed1c50df: Implement artifact release for Harbormaster
Summary

Resolves T5836. This automatically releases artifacts when Harbormaster builds finish (either passing or failing). This allows Harbormaster to release the Drydock leases it has for hosts.

Test Plan

Tested it with a build plan that passes and fails; tested it with lots of builds running in parallel.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

hach-que retitled this revision from to Implement artifact release for 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 a task: Restricted Maniphest Task.
epriestley edited edge metadata.

I think we don't need a lock for this, and we can do this outside of the lock (and a little more simply, see inlines). Does that make sense? Is the a reason the lock is important?

This basically looks fine to me.

It might also be nice to have a bin/harbormaster release-artifact <id> or something to make debugging easier.

At some point maybe the artifacts should track their acquisition state, too. That can happen in the future, though.

src/applications/harbormaster/engine/HarbormasterBuildEngine.php
65

I think we should do this after releasing the lock..?

76

Can we cover the pass/fail/deadlocked cases here (also after lock release) instead of inside updateBuildSteps()?

src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
128–144

Fine, but also yuck -- I think we need ArtifactType kinda-soon-ish.

This revision now requires changes to proceed.Aug 11 2014, 7:55 PM

Oh, I didn't see that lock there. There's no reason for this to be in any locks.

hach-que edited edge metadata.

Changes requested in code review

This revision is now accepted and ready to land.Aug 11 2014, 11:11 PM
hach-que updated this revision to Diff 24609.

Closed by commit rPaab0ed1c50df (authored by @hach-que).