Implement "Upload Artifact" build step
ClosedPublic
Actions

Authored by hach-que on Nov 13 2013, 9:37 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T1049: Implement Harbormaster

Commits

Restricted Diffusion Commit
rPdd01535ed6d7: Implement "Upload Artifact" build step

Summary

This implements a build step for uploading an artifact from a build machine to Phabricator. It uses SFTP so that it will work on both UNIX and Windows build machines.

Test Plan

Ran an "Upload Artifact" build against a Windows machine (with FreeSSHD installed). The artifact uploaded to Phabricator, appeared on the build view and the file contents could be viewed from Phabricator.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

Fix line length issues

hach-que added inline comments.Nov 13 2013, 9:46 PM

src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
51–54	Dat N+1. But in all serious we would have to make the logic more complex to eradicate this (first pass to find all PHIDs in all types, load them all in one go and return the whole list from some static function). In reality, someone would need to have hundreds of file build artifacts before this would be an issue, and I think the much more common case will be "zip the contents up on the build server, upload the ZIP as the build artifact".

Before we pursue this, I think we should get BuildTarget up and running. The idea with BuildTarget is that it's a concrete version of a BuildStep. The purpose is twofold:

Capture the build step as it existed when it was run. For example, this gives us a place to store what all the values of the variables were, so you can later look back and figure out what happened. Although all the variables are relatively deterministic and reproducible for now, this is unlikely to hold forever. In the future, other types of data and relationships will be mutable after the build step runs (like dependency information, etc). Even now, you can change the configuration and this essentially applies retroactively to all existing build steps which have already run, which may be confusing.
Potentially, allow a build step to generate multiple targets. I think the common case here is probably literal build target (x86, x86_64) where you want to parameterize the step and build out some N-dimensional matrix by permuting one or more vectors of options. If you target two architectures, three OSes, and two environments, that's already 12 build steps if they aren't parameterizable. I think this is a far-future capability which we may never actually implement, but that it's good to leave room in the design for it so we have it available if we need it.

In practice, this just means:

When we run a BuildStep, we create a BuildTarget for it before doing the actual run.
Artifacts point at BuildTargets instead of BuildSteps.
Logs also point at BuildTargets instead of BuildSteps.

The BuildTarget doesn't have to have anything interesting or useful in it for now.

src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php
52	This is missing a `-i %P`?
115	Pass the name in options (the second parameter).
125	Use `Filesystem::readRandomCharacters()`, but I think the build step should have a config option which name the artifact, which we use here. For example: Artifact Name: x86_build.exe ...would `setArtifactKey("x86_build.exe")`. This allows other build steps to refer to this artifact. (This could maybe be optional and use the basename of the file if omitted.) Or just having the "Local Name" determine this, if present, is probably cleanest. The: UNIQUE KEY `key_artifact_type` (`artifactType`,`artifactIndex`), ...that I put on the table is also probably a really dumb idea and should be removed. This one might be dumb too: UNIQUE KEY `key_artifact` (`buildablePHID`,`artifactType`,`artifactIndex`), Maybe that should be: `buildStepPHID`, `artifactType`, `artifactIndex` ...instead. In all cases, the goal is just to make sure each artifact can be uniquely identified.
src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
51–54	This is fine for now. Long term we'll probably move all the types to separate classes (so third parties could add new types) and give those implementations some `getRequiredHandlePHIDs()`, plus move the rendering there. This is reasonable in the meantime, though.

Clarified on IRC:

The daemon begins executing a BuildStep; the first thing it does is create BuildTarget(s) for the build step; then it executes those Target(s).

Holding off work on this until T4111 and Passphrase are finalised since both of those will dramatically change how this build step works.

This change updates "Upload Artifact" to be based on the latest changes (Drydock-based hosts). I've verified that this works on both Linux and Windows remote hosts.

It also includes a minor fix in DrydockBlueprintQuery which I noticed while trying to create a new blueprint.