@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Make Harbormaster input and output artifacts more explicit
Summary:
Ref T1049. In Harbormaster, build steps may have various inputs (like a host they should run on) and outputs (like a reference to an uploaded file).
- Currently, inputs aren't defined anywhere (except implicitly at runtime).
- Instead, define inputs explicitly.
- Currently, outputs are defined in a way that loses information when misconfigured (the keys will collide).
- Instead, define inputs and outputs so they work whether a step is configured correctly or not.
- Currently, there's no simple way to see a step's inputs and outputs.
- Add some UI for this.
- Currently, reordering steps has some surprising side effects.
- Instead of invalidating steps after reordering them, validate them at display time and warn the user.
Test Plan:
{F133679}
{F133680}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, chad
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D8599
+251
-70
+2
resources/celerity/map.php
+2
resources/celerity/map.php
···
70
70
'rsrc/css/application/feed/feed.css' => '0d17c209',
71
71
'rsrc/css/application/files/global-drag-and-drop.css' => '697324ad',
72
72
'rsrc/css/application/flag/flag.css' => '5337623f',
73
+
'rsrc/css/application/harbormaster/harbormaster.css' => 'cec833b7',
73
74
'rsrc/css/application/herald/herald-test.css' => '2b7d0f54',
74
75
'rsrc/css/application/herald/herald.css' => '59d48f01',
75
76
'rsrc/css/application/maniphest/batch-editor.css' => '8f380ebc',
···
521
522
'diviner-shared-css' => '38813222',
522
523
'font-source-sans-pro' => '225851dd',
523
524
'global-drag-and-drop-css' => '697324ad',
525
+
'harbormaster-css' => 'cec833b7',
524
526
'herald-css' => '59d48f01',
525
527
'herald-rule-editor' => '4173dbd8',
526
528
'herald-test-css' => '2b7d0f54',
+2
-33
src/applications/harbormaster/controller/HarbormasterPlanOrderController.php
+2
-33
src/applications/harbormaster/controller/HarbormasterPlanOrderController.php
···
1
1
<?php
2
2
3
-
/**
4
-
* @group search
5
-
*/
6
3
final class HarbormasterPlanOrderController extends HarbormasterController {
7
4
8
5
private $id;
···
46
43
$reordered_steps[] = $step;
47
44
}
48
45
49
-
// We must ensure that steps with artifacts become invalid if they are
50
-
// placed before the steps that produce them.
51
-
foreach ($reordered_steps as $step) {
52
-
$implementation = $step->getStepImplementation();
53
-
$settings = $implementation->getSettings();
54
-
foreach ($implementation->getSettingDefinitions() as $name => $opt) {
55
-
switch ($opt['type']) {
56
-
case BuildStepImplementation::SETTING_TYPE_ARTIFACT:
57
-
$value = $settings[$name];
58
-
$filter = $opt['artifact_type'];
59
-
$available_artifacts =
60
-
BuildStepImplementation::getAvailableArtifacts(
61
-
$plan,
62
-
$reordered_steps,
63
-
$step,
64
-
$filter);
65
-
$artifact_found = false;
66
-
foreach ($available_artifacts as $key => $type) {
67
-
if ($key === $value) {
68
-
$artifact_found = true;
69
-
}
70
-
}
71
-
if (!$artifact_found) {
72
-
$step->setDetail($name, null);
73
-
}
74
-
break;
75
-
}
76
-
$step->save();
77
-
}
78
-
}
46
+
// NOTE: Reordering steps may invalidate artifacts. This is fine; the UI
47
+
// will show that there are ordering issues.
79
48
80
49
// Force the page to re-render.
81
50
return id(new AphrontRedirectResponse());
+167
-21
src/applications/harbormaster/controller/HarbormasterPlanViewController.php
+167
-21
src/applications/harbormaster/controller/HarbormasterPlanViewController.php
···
50
50
$crumbs = $this->buildApplicationCrumbs();
51
51
$crumbs->addTextCrumb(pht("Plan %d", $id));
52
52
53
-
$step_list = $this->buildStepList($plan);
53
+
list($step_list, $has_any_conflicts) = $this->buildStepList($plan);
54
+
55
+
if ($has_any_conflicts) {
56
+
$box->setFormErrors(
57
+
array(
58
+
pht(
59
+
'This build plan has conflicts in one or more build steps. '.
60
+
'Examine the step list and resolve the listed errors.'),
61
+
));
62
+
}
54
63
55
64
return $this->buildApplicationPage(
56
65
array(
···
91
100
'listID' => $list_id,
92
101
'orderURI' => '/harbormaster/plan/order/'.$plan->getID().'/',
93
102
));
103
+
104
+
$has_any_conflicts = false;
94
105
foreach ($steps as $step) {
95
106
$implementation = null;
96
107
try {
···
121
132
->setObjectName("Step ".$i++)
122
133
->setHeader($implementation->getName());
123
134
124
-
if (!$implementation->validateSettings()) {
125
-
$item
126
-
->setBarColor('red')
127
-
->addAttribute(pht('This step is not configured correctly.'));
128
-
} else {
129
-
$item->addAttribute($implementation->getDescription());
130
-
}
135
+
$item->addAttribute($implementation->getDescription());
136
+
137
+
$step_id = $step->getID();
138
+
$edit_uri = $this->getApplicationURI("step/edit/{$step_id}/");
139
+
$delete_uri = $this->getApplicationURI("step/delete/{$step_id}/");
131
140
132
141
if ($can_edit) {
133
-
$edit_uri = $this->getApplicationURI("step/edit/".$step->getID()."/");
134
-
$item
135
-
->setHref($edit_uri)
136
-
->addAction(
137
-
id(new PHUIListItemView())
138
-
->setIcon('delete')
139
-
->addSigil('harbormaster-build-step-delete')
140
-
->setWorkflow(true)
141
-
->setRenderNameAsTooltip(true)
142
-
->setName(pht("Delete"))
143
-
->setHref(
144
-
$this->getApplicationURI("step/delete/".$step->getID()."/")));
142
+
$item->setHref($edit_uri);
145
143
$item->setGrippable(true);
146
144
$item->addSigil('build-step');
147
145
$item->setMetadata(
···
150
148
));
151
149
}
152
150
151
+
$item
152
+
->setHref($edit_uri)
153
+
->addAction(
154
+
id(new PHUIListItemView())
155
+
->setIcon('delete')
156
+
->addSigil('harbormaster-build-step-delete')
157
+
->setWorkflow(true)
158
+
->setDisabled(!$can_edit)
159
+
->setHref(
160
+
$this->getApplicationURI("step/delete/".$step->getID()."/")));
161
+
162
+
$inputs = $step->getStepImplementation()->getArtifactInputs();
163
+
$outputs = $step->getStepImplementation()->getArtifactOutputs();
164
+
165
+
$has_conflicts = false;
166
+
if ($inputs || $outputs) {
167
+
$available_artifacts = BuildStepImplementation::loadAvailableArtifacts(
168
+
$plan,
169
+
$step,
170
+
null);
171
+
172
+
list($inputs_ui, $has_conflicts) = $this->buildArtifactList(
173
+
$inputs,
174
+
'in',
175
+
pht('Input Artifacts'),
176
+
$available_artifacts);
177
+
178
+
list($outputs_ui) = $this->buildArtifactList(
179
+
$outputs,
180
+
'out',
181
+
pht('Output Artifacts'),
182
+
array());
183
+
184
+
$item->appendChild(
185
+
phutil_tag(
186
+
'div',
187
+
array(
188
+
'class' => 'harbormaster-artifact-io',
189
+
),
190
+
array(
191
+
$inputs_ui,
192
+
$outputs_ui,
193
+
)));
194
+
}
195
+
196
+
if ($has_conflicts) {
197
+
$has_any_conflicts = true;
198
+
$item->setBarColor('red');
199
+
}
200
+
153
201
$step_list->addItem($item);
154
202
}
155
203
156
-
return $step_list;
204
+
return array($step_list, $has_any_conflicts);
157
205
}
158
206
159
207
private function buildActionList(HarbormasterBuildPlan $plan) {
···
231
279
pht('Created'),
232
280
phabricator_datetime($plan->getDateCreated(), $viewer));
233
281
282
+
}
283
+
284
+
private function buildArtifactList(
285
+
array $artifacts,
286
+
$kind,
287
+
$name,
288
+
array $available_artifacts) {
289
+
$has_conflicts = false;
290
+
291
+
if (!$artifacts) {
292
+
return array(null, $has_conflicts);
293
+
}
294
+
295
+
296
+
$this->requireResource('harbormaster-css');
297
+
298
+
$header = phutil_tag(
299
+
'div',
300
+
array(
301
+
'class' => 'harbormaster-artifact-summary-header',
302
+
),
303
+
$name);
304
+
305
+
$is_input = ($kind == 'in');
306
+
307
+
$list = new PHUIStatusListView();
308
+
foreach ($artifacts as $artifact) {
309
+
$error = null;
310
+
311
+
$key = idx($artifact, 'key');
312
+
if (!strlen($key)) {
313
+
$bound = phutil_tag('em', array(), pht('(null)'));
314
+
if ($is_input) {
315
+
// This is an unbound input. For now, all inputs are always required.
316
+
$icon = 'warning-red';
317
+
$icon_label = pht('Required Input');
318
+
$has_conflicts = true;
319
+
$error = pht('This input is required, but not configured.');
320
+
} else {
321
+
// This is an unnamed output. Outputs do not necessarily need to be
322
+
// named.
323
+
$icon = 'open';
324
+
$icon_label = pht('Unused Output');
325
+
}
326
+
} else {
327
+
$bound = phutil_tag('strong', array(), $key);
328
+
if ($is_input) {
329
+
if (isset($available_artifacts[$key])) {
330
+
if ($available_artifacts[$key] == idx($artifact, 'type')) {
331
+
$icon = 'accept-green';
332
+
$icon_label = pht('Valid Input');
333
+
} else {
334
+
$icon = 'warning-red';
335
+
$icon_label = pht('Bad Input Type');
336
+
$has_conflicts = true;
337
+
$error = pht(
338
+
'This input is bound to the wrong artifact type. It is bound '.
339
+
'to a "%s" artifact, but should be bound to a "%s" artifact.',
340
+
$available_artifacts[$key],
341
+
idx($artifact, 'type'));
342
+
}
343
+
} else {
344
+
$icon = 'question-red';
345
+
$icon_label = pht('Unknown Input');
346
+
$has_conflicts = true;
347
+
$error = pht(
348
+
'This input is bound to an artifact ("%s") which does not exist '.
349
+
'at this stage in the build process.',
350
+
$key);
351
+
}
352
+
} else {
353
+
$icon = 'down-green';
354
+
$icon_label = pht('Valid Output');
355
+
}
356
+
}
357
+
358
+
if ($error) {
359
+
$note = array(
360
+
phutil_tag('strong', array(), pht('ERROR:')),
361
+
' ',
362
+
$error);
363
+
} else {
364
+
$note = $bound;
365
+
}
366
+
367
+
$list->addItem(
368
+
id(new PHUIStatusItemView())
369
+
->setIcon($icon, $icon_label)
370
+
->setTarget($artifact['name'])
371
+
->setNote($note));
372
+
}
373
+
374
+
$ui = array(
375
+
$header,
376
+
$list,
377
+
);
378
+
379
+
return array($ui, $has_conflicts);
234
380
}
235
381
236
382
}
+11
-2
src/applications/harbormaster/step/BuildStepImplementation.php
+11
-2
src/applications/harbormaster/step/BuildStepImplementation.php
···
50
50
return $this->settings;
51
51
}
52
52
53
+
public function getSetting($key, $default = null) {
54
+
return idx($this->settings, $key, $default);
55
+
}
56
+
53
57
/**
54
58
* Validate the current settings of this build step.
55
59
*/
···
103
107
*
104
108
* @return array The mappings of artifact names to their types.
105
109
*/
106
-
public function getArtifactMappings() {
110
+
public function getArtifactInputs() {
111
+
return array();
112
+
}
113
+
114
+
public function getArtifactOutputs() {
107
115
return array();
108
116
}
109
117
···
141
149
$previous_implementations[] = $build_step->getStepImplementation();
142
150
}
143
151
144
-
$artifact_arrays = mpull($previous_implementations, 'getArtifactMappings');
152
+
$artifact_arrays = mpull($previous_implementations, 'getArtifactOutputs');
145
153
$artifacts = array();
146
154
foreach ($artifact_arrays as $array) {
155
+
$array = ipull($array, 'type', 'key');
147
156
foreach ($array as $name => $type) {
148
157
if ($type !== $artifact_type && $artifact_type !== null) {
149
158
continue;
+10
src/applications/harbormaster/step/CommandBuildStepImplementation.php
+10
src/applications/harbormaster/step/CommandBuildStepImplementation.php
···
90
90
return true;
91
91
}
92
92
93
+
public function getArtifactInputs() {
94
+
return array(
95
+
array(
96
+
'name' => pht('Run on Host'),
97
+
'key' => $this->getSetting('hostartifact'),
98
+
'type' => HarbormasterBuildArtifact::TYPE_HOST,
99
+
),
100
+
);
101
+
}
102
+
93
103
public function getSettingDefinitions() {
94
104
return array(
95
105
'command' => array(
+10
-7
src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php
+10
-7
src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php
···
51
51
$artifact->save();
52
52
}
53
53
54
-
public function getArtifactMappings() {
55
-
$settings = $this->getSettings();
56
-
57
-
return array(
58
-
$settings['name'] => HarbormasterBuildArtifact::TYPE_HOST);
59
-
}
60
-
61
54
public function validateSettings() {
62
55
$settings = $this->getSettings();
63
56
···
69
62
}
70
63
71
64
return true;
65
+
}
66
+
67
+
public function getArtifactOutputs() {
68
+
return array(
69
+
array(
70
+
'name' => pht('Leased Host'),
71
+
'key' => $this->getSetting('name'),
72
+
'type' => HarbormasterBuildArtifact::TYPE_HOST,
73
+
),
74
+
);
72
75
}
73
76
74
77
public function getSettingDefinitions() {
+10
src/applications/harbormaster/step/PublishFragmentBuildStepImplementation.php
+10
src/applications/harbormaster/step/PublishFragmentBuildStepImplementation.php
···
73
73
return true;
74
74
}
75
75
76
+
public function getArtifactInputs() {
77
+
return array(
78
+
array(
79
+
'name' => pht('Publishes File'),
80
+
'key' => $this->getSetting('artifact'),
81
+
'type' => HarbormasterBuildArtifact::TYPE_FILE,
82
+
),
83
+
);
84
+
}
85
+
76
86
public function getSettingDefinitions() {
77
87
return array(
78
88
'path' => array(
+20
-7
src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php
+20
-7
src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php
···
51
51
$artifact->save();
52
52
}
53
53
54
-
public function getArtifactMappings() {
55
-
$settings = $this->getSettings();
56
-
57
-
return array(
58
-
$settings['name'] => HarbormasterBuildArtifact::TYPE_FILE);
59
-
}
60
-
61
54
public function validateSettings() {
62
55
$settings = $this->getSettings();
63
56
···
75
68
// TODO: Check if the host artifact is provided by previous build steps.
76
69
77
70
return true;
71
+
}
72
+
73
+
public function getArtifactInputs() {
74
+
return array(
75
+
array(
76
+
'name' => pht('Upload From Host'),
77
+
'key' => $this->getSetting('hostartifact'),
78
+
'type' => HarbormasterBuildArtifact::TYPE_HOST,
79
+
),
80
+
);
81
+
}
82
+
83
+
public function getArtifactOutputs() {
84
+
return array(
85
+
array(
86
+
'name' => pht('Uploaded File'),
87
+
'key' => $this->getSetting('name'),
88
+
'type' => HarbormasterBuildArtifact::TYPE_FILE,
89
+
),
90
+
);
78
91
}
79
92
80
93
public function getSettingDefinitions() {
+19
webroot/rsrc/css/application/harbormaster/harbormaster.css
+19
webroot/rsrc/css/application/harbormaster/harbormaster.css
···
1
+
/**
2
+
* @provides harbormaster-css
3
+
*/
4
+
5
+
.harbormaster-artifact-io {
6
+
margin: 0 0 0 8px;
7
+
padding: 4px 8px;
8
+
border-width: 1px 0 0 1px;
9
+
border-style: solid;
10
+
box-shadow: inset 2px 2px 1px rgba(0, 0, 0, 0.075);
11
+
background: {$lightbluebackground};
12
+
border-color: {$lightblueborder};
13
+
}
14
+
15
+
.harbormaster-artifact-summary-header {
16
+
font-weight: bold;
17
+
margin-bottom: 2px;
18
+
color: {$darkbluetext};
19
+
}