Skip to content

Commit 4c29e85

Browse files
yk634yk634
authored
feat(3486): validate that requires only reference existing stages (#185)
1 parent 26068e4 commit 4c29e85

9 files changed

Lines changed: 276 additions & 7 deletions

‎lib/phase/flatten.js‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
const Hoek = require('@hapi/hoek');
44
const clone = require('clone');
55
const { STAGE_TRIGGER } = require('screwdriver-data-schema/config/regex');
6+
const { normalizeRequires } = require('./requires');
67

78
const STAGE_PREFIX = 'stage@';
89
const STAGE_SETUP_TEARDOWN_PATTERN = /stage@[\w-]+:(?:setup|teardown)$/;
@@ -628,11 +629,7 @@ function getTeardownRequires(jobs, stageName) {
628629
jobs[jobName].stage.name === stageName &&
629630
!STAGE_SETUP_TEARDOWN_PATTERN.test(jobName)
630631
) {
631-
if (Array.isArray(jobs[jobName].requires)) {
632-
stageJobsRequires = stageJobsRequires.concat(jobs[jobName].requires);
633-
} else {
634-
stageJobsRequires.push(jobs[jobName].requires);
635-
}
632+
stageJobsRequires = stageJobsRequires.concat(normalizeRequires(jobs[jobName].requires));
636633
stageJobNames.push(jobName);
637634
}
638635
});

‎lib/phase/functional.js‎

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
const Joi = require('joi');
44
const Hoek = require('@hapi/hoek');
55
const clone = require('clone');
6+
const { STAGE_TRIGGER, STAGE_SETUP_TEARDOWN_JOB_NAME } = require('screwdriver-data-schema/config/regex');
67
const WorkflowParser = require('screwdriver-workflow-parser');
78
const SlackValidation = require('screwdriver-notifications-slack');
89
const EmailValidation = require('screwdriver-notifications-email');
10+
const { normalizeRequires } = require('./requires');
911

1012
// Actual environment size is limited by space, not quantity.
1113
// We do this to force sd.yaml to be simpler.
@@ -242,19 +244,78 @@ async function generateWorkflowGraph(doc, triggerFactory, pipelineId) {
242244
return WorkflowParser.getWorkflow(cloneDoc, triggerFactory, pipelineId);
243245
}
244246

247+
/**
248+
* Validate that requires fields only reference existing stages
249+
* @method validateRequiresStagesExist
250+
* @param {Object} config
251+
* @param {Object} config.jobs Map of job name to job configuration
252+
* @param {Object} config.stages Map of stage name to stage configuration
253+
* @return {Array} List of errors
254+
*/
255+
function validateRequiresStagesExist({ jobs, stages }) {
256+
const errors = [];
257+
const stageNames = Object.keys(stages || {});
258+
const stageNameSet = new Set(stageNames);
259+
260+
// Validate job-level requires that reference stages.
261+
Object.entries(jobs || {}).forEach(([jobName, job]) => {
262+
const setupTeardownMatched = STAGE_SETUP_TEARDOWN_JOB_NAME.exec(jobName);
263+
264+
// Stage-level requires are copied to setup jobs during flattening, so skip them here.
265+
if (setupTeardownMatched && setupTeardownMatched[2] === 'setup') {
266+
return;
267+
}
268+
269+
const requiresList = normalizeRequires(job.requires);
270+
271+
requiresList.forEach(requires => {
272+
const matched = STAGE_TRIGGER.exec(requires);
273+
274+
if (matched && !stageNameSet.has(matched[1])) {
275+
errors.push(
276+
`${jobName} job has invalid requires: ${requires}. Cannot find stage ${matched[1]} in stages.`
277+
);
278+
}
279+
});
280+
});
281+
282+
// Validate stage-level requires that reference stages.
283+
Object.entries(stages || {}).forEach(([stageName, stage]) => {
284+
const stageRequiresList = normalizeRequires(stage.requires);
285+
286+
stageRequiresList.forEach(requires => {
287+
const matched = STAGE_TRIGGER.exec(requires);
288+
289+
if (matched && !stageNameSet.has(matched[1])) {
290+
errors.push(
291+
`${stageName} stage has invalid requires: ${requires}. Cannot find stage ${matched[1]} in stages.`
292+
);
293+
}
294+
});
295+
});
296+
297+
return errors;
298+
}
299+
245300
/**
246301
* Ensure the workflowGraph is a valid one
302+
* - Stage references point to existing stages
247303
* - No cycles in workflowGraph
248304
* @method validateWorkflowGraph
249305
* @param {Object} doc Document that went through flattening
250306
* @return {Array} List of errors
251307
*/
252308
function validateWorkflowGraph(doc) {
309+
const errors = validateRequiresStagesExist({
310+
jobs: doc.jobs,
311+
stages: doc.stages
312+
});
313+
253314
if (WorkflowParser.hasCycle(doc.workflowGraph)) {
254-
return ['Jobs: should not have circular dependency in jobs'];
315+
errors.push('Jobs: should not have circular dependency in jobs');
255316
}
256317

257-
return [];
318+
return errors;
258319
}
259320

260321
/**

‎lib/phase/requires.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
'use strict';
2+
3+
/**
4+
* Normalize requires value to array
5+
* @method normalizeRequires
6+
* @param {String|Array|undefined|null} requiresValue Value to normalize
7+
* @return {Array}
8+
*/
9+
function normalizeRequires(requiresValue) {
10+
if (Array.isArray(requiresValue)) {
11+
return requiresValue;
12+
}
13+
14+
if (!requiresValue) {
15+
return [];
16+
}
17+
18+
return [requiresValue];
19+
}
20+
21+
module.exports = {
22+
normalizeRequires
23+
};

‎test/data/pipeline-with-requires-existing-stages.yaml‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
jobs:
2+
job1:
3+
image: node:18
4+
requires: []
5+
steps:
6+
- echo: echo job1
7+
job2:
8+
image: node:18
9+
requires: [~job1]
10+
steps:
11+
- echo: echo job2
12+
job3:
13+
image: node:18
14+
requires: [stage@stg1]
15+
steps:
16+
- echo: echo job3
17+
job4:
18+
image: node:18
19+
requires: [~stage@stg1]
20+
steps:
21+
- echo: echo job4
22+
stages:
23+
stg1:
24+
jobs: [job1, job2]
25+
requires: [~commit]

‎test/data/pipeline-with-requires-invalid-stages.yaml‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
jobs:
2+
job1:
3+
image: node:18
4+
requires: []
5+
steps:
6+
- echo: echo job1
7+
job2:
8+
image: node:18
9+
requires: [~job1]
10+
steps:
11+
- echo: echo job2
12+
job3:
13+
image: node:18
14+
requires: [~stage@stg2]
15+
steps:
16+
- echo: echo job3
17+
job4:
18+
image: node:18
19+
requires: [stage@stg1, stage@stg2]
20+
steps:
21+
- echo: echo job4
22+
stages:
23+
stg1:
24+
jobs: [job1, job2]
25+
requires: [~commit]

‎test/data/pipeline-with-requires-nonexistent-stage.yaml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
jobs:
2+
job1:
3+
image: node:18
4+
requires: []
5+
steps:
6+
- echo: echo job1
7+
job2:
8+
image: node:18
9+
requires: [~job1]
10+
steps:
11+
- echo: echo job2
12+
job3:
13+
image: node:18
14+
requires: [stage@stg1]
15+
steps:
16+
- echo: echo job3

‎test/data/pipeline-with-stage-requires-existing-stages.yaml‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
jobs:
2+
job1:
3+
image: node:18
4+
requires: []
5+
steps:
6+
- echo: echo job1
7+
job2:
8+
image: node:18
9+
requires: []
10+
steps:
11+
- echo: echo job2
12+
job3:
13+
image: node:18
14+
requires: []
15+
steps:
16+
- echo: echo job3
17+
stages:
18+
stg1:
19+
jobs: [job1]
20+
requires: [~commit]
21+
stg2:
22+
jobs: [job2]
23+
requires: [stage@stg1]
24+
stg3:
25+
jobs: [job3]
26+
requires: [~stage@stg1]

‎test/data/pipeline-with-stage-requires-invalid-stages.yaml‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
jobs:
2+
job1:
3+
image: node:18
4+
requires: []
5+
steps:
6+
- echo: echo job1
7+
job2:
8+
image: node:18
9+
requires: []
10+
steps:
11+
- echo: echo job2
12+
job3:
13+
image: node:18
14+
requires: []
15+
steps:
16+
- echo: echo job3
17+
job4:
18+
image: node:18
19+
requires: []
20+
steps:
21+
- echo: echo job4
22+
stages:
23+
stg1:
24+
jobs: [job1]
25+
requires: [~commit]
26+
stg2:
27+
jobs: [job2]
28+
requires: [stage@stg1]
29+
stg3:
30+
jobs: [job3]
31+
requires: [stage@stg1, stage@missing1]
32+
stg4:
33+
jobs: [job4]
34+
requires: [~stage@missing2]

‎test/index.test.js‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,68 @@ describe('config parser', () => {
285285
}).then(data => {
286286
assert.match(data.errors[0], /Error: Job foo in stage cannot have sourcePaths defined./);
287287
}));
288+
289+
it('returns an error if a job requires a nonexistent stage and no stages are defined', () =>
290+
parser({
291+
yaml: loadData('pipeline-with-requires-nonexistent-stage.yaml'),
292+
triggerFactory,
293+
pipelineId
294+
}).then(data => {
295+
assert.match(
296+
data.errors[0],
297+
/Error: job3 job has invalid requires: stage@stg1\. Cannot find stage stg1 in stages\./
298+
);
299+
}));
300+
301+
it('returns errors if jobs require nonexistent stages when stages are defined', () =>
302+
parser({
303+
yaml: loadData('pipeline-with-requires-invalid-stages.yaml'),
304+
triggerFactory,
305+
pipelineId
306+
}).then(data => {
307+
assert.match(
308+
data.errors[0],
309+
/Error: job3 job has invalid requires: ~stage@stg2:teardown\. Cannot find stage stg2 in stages\./
310+
);
311+
assert.match(
312+
data.errors[0],
313+
/job4 job has invalid requires: stage@stg2:teardown\. Cannot find stage stg2 in stages\./
314+
);
315+
}));
316+
317+
it('does not return an error if jobs require existing stages', () =>
318+
parser({
319+
yaml: loadData('pipeline-with-requires-existing-stages.yaml'),
320+
triggerFactory,
321+
pipelineId
322+
}).then(data => {
323+
assert.notOk(data.errors);
324+
}));
325+
326+
it('returns errors if stages require nonexistent stages', () =>
327+
parser({
328+
yaml: loadData('pipeline-with-stage-requires-invalid-stages.yaml'),
329+
triggerFactory,
330+
pipelineId
331+
}).then(data => {
332+
assert.match(
333+
data.errors[0],
334+
/Error: stg3 stage has invalid requires: stage@missing1:teardown\. Cannot find stage missing1 in stages\./
335+
);
336+
assert.match(
337+
data.errors[0],
338+
/stg4 stage has invalid requires: ~stage@missing2:teardown\. Cannot find stage missing2 in stages\./
339+
);
340+
}));
341+
342+
it('does not return an error if stages require existing stages', () =>
343+
parser({
344+
yaml: loadData('pipeline-with-stage-requires-existing-stages.yaml'),
345+
triggerFactory,
346+
pipelineId
347+
}).then(data => {
348+
assert.notOk(data.errors);
349+
}));
288350
});
289351

290352
describe('subscribe', () => {

0 commit comments

Comments
 (0)