Skip to content

Commit d6ce6dc

Browse files
joyeecheungjoyeecheung
authored
lib: clean up PR and CI url parsing logic (nodejs#242)
- Put the PR URL parsing logic in links.js and reuse it - Differentiate PR and commit jobs in preparation for nodejs#161 - Updates a few CI link patterns
1 parent b7b2a8f commit d6ce6dc

File tree

10 files changed

+224
-116
lines changed

10 files changed

+224
-116
lines changed

‎.eslintrc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
"space-before-function-paren": ["error", "never"],
66
"no-multi-spaces": ["error", { "ignoreEOLComments": true }],
77
"camelcase": "off",
8-
"max-len": [2, 80, 4, {"ignoreUrls": true}]
8+
"max-len": [2, 80, 4, {"ignoreUrls": true}],
9+
"object-property-newline": "off"
910
},
1011
"env": {
1112
"node": true

‎components/git/land.js‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
'use strict';
22

3+
const { parsePRFromURL } = require('../../lib/links');
34
const getMetadata = require('../metadata');
45
const CLI = require('../../lib/cli');
56
const Request = require('../../lib/request');
@@ -59,30 +60,35 @@ const FINAL = 'final';
5960
const CONTINUE = 'continue';
6061
const ABORT = 'abort';
6162

62-
const GITHUB_PULL_REQUEST_URL = /github.com\/[^/]+\/[^/]+\/pull\/(\d+)/;
63-
6463
function handler(argv) {
65-
if (argv.prid &&
66-
(Number.isInteger(argv.prid) || argv.prid.match(GITHUB_PULL_REQUEST_URL))
67-
) {
68-
return land(START, argv);
64+
if (argv.prid) {
65+
if (Number.isInteger(argv.prid)) {
66+
return land(START, argv);
67+
} else {
68+
const parsed = parsePRFromURL(argv.prid);
69+
if (parsed) {
70+
Object.assign(argv, parsed);
71+
return land(START, argv);
72+
}
73+
}
74+
yargs.showHelp();
75+
return;
6976
}
77+
7078
const provided = [];
7179
for (const type of Object.keys(landOptions)) {
7280
if (argv[type]) {
7381
provided.push(type);
7482
}
7583
}
76-
if (provided.length === 0) {
77-
yargs.showHelp();
78-
return;
79-
}
80-
if (provided.length > 1) {
81-
yargs.showHelp();
82-
return;
84+
85+
if (provided.length === 1) {
86+
return land(provided[0], argv);
8387
}
8488

85-
return land(provided[0], argv);
89+
// If the more than one action is provided or no valid action
90+
// is provided, show help.
91+
yargs.showHelp();
8692
}
8793

8894
function land(state, argv) {
@@ -129,10 +135,6 @@ async function main(state, argv, cli, req, dir) {
129135
}
130136

131137
if (state === START) {
132-
if (argv.prid.match && argv.prid.match(GITHUB_PULL_REQUEST_URL)) {
133-
argv.prid = Number(argv.prid.split('/').pop());
134-
}
135-
136138
if (session.hasStarted()) {
137139
cli.warn(
138140
'Previous `git node land` session for ' +

‎components/git/metadata.js‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
const yargs = require('yargs');
44

5+
const { parsePRFromURL } = require('../../lib/links');
56
const getMetadata = require('../metadata');
67
const CLI = require('../../lib/cli');
78
const config = require('../../lib/config').getMergedConfig();
@@ -59,25 +60,16 @@ function builder(yargs) {
5960
.wrap(90);
6061
}
6162

62-
const PR_RE = new RegExp(
63-
'^https://github.com/(\\w+)/([a-zA-Z.-]+)/pull/' +
64-
'([0-9]+)(?:/(?:files)?)?$');
65-
6663
function handler(argv) {
67-
// remove hashes from PR link
68-
argv.identifier = argv.identifier.replace(/#.*$/, '');
69-
70-
const parsed = {};
64+
let parsed = {};
7165
const prid = Number.parseInt(argv.identifier);
7266
if (!Number.isNaN(prid)) {
7367
parsed.prid = prid;
74-
} else if (PR_RE.test(argv.identifier)) {
75-
const match = argv.identifier.match(PR_RE);
76-
parsed.owner = match[1];
77-
parsed.repo = match[2];
78-
parsed.prid = Number.parseInt(match[3]);
7968
} else {
80-
return yargs.showHelp();
69+
parsed = parsePRFromURL(argv.identifier);
70+
if (!parsed) {
71+
return yargs.showHelp();
72+
}
8173
}
8274

8375
if (!Number.isInteger(argv.maxCommits) || argv.maxCommits < 0) {

‎lib/ci.js‎

Lines changed: 70 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,66 @@
11
'use strict';
22

3-
// (XXX) Do we need the protocol?
4-
const CI_RE = /https:\/\/ci\.nodejs\.org(\S+)/mg;
3+
const CI_URL_RE = /\/\/ci\.nodejs\.org(\S+)/mg;
54
const CI_DOMAIN = 'ci.nodejs.org';
65

76
// constants
87
const CITGM = 'CITGM';
9-
const FULL = 'FULL';
8+
const PR = 'PR';
9+
const COMMIT = 'COMMIT';
1010
const BENCHMARK = 'BENCHMARK';
1111
const LIBUV = 'LIBUV';
1212
const NOINTL = 'NOINTL';
1313
const V8 = 'V8';
1414
const LINTER = 'LINTER';
15-
const LITE = 'LITE';
15+
const LITE_PR = 'LITE_PR';
16+
const LITE_COMMIT = 'LITE_COMMIT';
1617

1718
const CI_TYPES = new Map([
18-
[CITGM, { name: 'CITGM', re: /citgm/ }],
19-
[FULL,
20-
{ name: 'Full',
21-
re: /node-test-pull-request\/|node-test-commit\// }],
22-
[BENCHMARK, { name: 'Benchmark', re: /benchmark/ }],
23-
[LIBUV, { name: 'libuv', re: /libuv/ }],
24-
[NOINTL, { name: 'No Intl', re: /nointl/ }],
25-
[V8, { name: 'V8', re: /node-test-commit-v8/ }],
26-
[LINTER, { name: 'Linter', re: /node-test-linter/ }],
27-
[LITE, { name: 'Lite', re: /node-test-.+-lite/ }]
19+
[CITGM, { name: 'CITGM', jobName: 'citgm-smoker' }],
20+
[PR, { name: 'Full PR', jobName: 'node-test-pull-request' }],
21+
[COMMIT, { name: 'Full Commit', jobName: 'node-test-commit' }],
22+
[BENCHMARK, {
23+
name: 'Benchmark',
24+
jobName: 'benchmark-node-micro-benchmarks'
25+
}],
26+
[LIBUV, { name: 'libuv', jobName: 'libuv-test-commit' }],
27+
[NOINTL, { name: 'No Intl', jobName: 'node-test-commit-nointl' }],
28+
[V8, { name: 'V8', jobName: 'node-test-commit-v8-linux' }],
29+
[LINTER, { name: 'Linter', jobName: 'node-test-linter' }],
30+
[LITE_PR, {
31+
name: 'Lite PR',
32+
jobName: 'node-test-pull-request-lite'
33+
}],
34+
[LITE_COMMIT, {
35+
name: 'Lite Commit',
36+
jobName: 'node-test-commit-lite'
37+
}]
2838
]);
2939

30-
class CIParser {
40+
function parseJobFromURL(url) {
41+
if (typeof url !== 'string') {
42+
return undefined;
43+
}
44+
45+
for (let [ type, info ] of CI_TYPES) {
46+
const re = new RegExp(`${CI_DOMAIN}/job/${info.jobName}/(\\d+)`);
47+
const match = url.match(re);
48+
if (match) {
49+
return {
50+
link: url,
51+
jobid: parseInt(match[1]),
52+
type: type
53+
};
54+
}
55+
}
56+
57+
return undefined;
58+
}
59+
60+
/**
61+
* Parse links of CI Jobs posted in a GitHub thread
62+
*/
63+
class JobParser {
3164
/**
3265
* @param {{bodyText: string, publishedAt: string}[]} thread
3366
*/
@@ -44,11 +77,15 @@ class CIParser {
4477
for (const c of thread) {
4578
const text = c.bodyText;
4679
if (!text.includes(CI_DOMAIN)) continue;
47-
const cis = this.parseText(text);
48-
for (const ci of cis) {
49-
const entry = result.get(ci.type);
80+
const jobs = this.parseText(text);
81+
for (const job of jobs) {
82+
const entry = result.get(job.type);
5083
if (!entry || entry.date < c.publishedAt) {
51-
result.set(ci.type, {link: ci.link, date: c.publishedAt});
84+
result.set(job.type, {
85+
link: job.link,
86+
date: c.publishedAt,
87+
jobid: job.jobid
88+
});
5289
}
5390
}
5491
}
@@ -57,30 +94,31 @@ class CIParser {
5794

5895
/**
5996
* @param {string} text
97+
* @returns {{link: string, jobid: number, type: string}}
6098
*/
6199
parseText(text) {
62-
const m = text.match(CI_RE);
63-
if (!m) {
100+
const links = text.match(CI_URL_RE);
101+
if (!links) {
64102
return [];
65103
}
66104

67105
const result = [];
68-
for (const link of m) {
69-
for (const [type, info] of CI_TYPES) {
70-
if (info.re.test(link)) {
71-
result.push({ type, link });
72-
break;
73-
}
106+
for (const link of links) {
107+
const parsed = parseJobFromURL(`https:${link}`);
108+
if (parsed) {
109+
result.push(parsed);
74110
}
75111
}
76112

77113
return result;
78114
}
79115
}
80116

81-
CIParser.TYPES = CI_TYPES;
82-
CIParser.constants = {
83-
CITGM, FULL, BENCHMARK, LIBUV, V8, NOINTL, LINTER, LITE
117+
module.exports = {
118+
JobParser,
119+
CI_TYPES,
120+
constants: {
121+
CITGM, PR, COMMIT, BENCHMARK, LIBUV, V8, NOINTL,
122+
LINTER, LITE_PR, LITE_COMMIT
123+
}
84124
};
85-
86-
module.exports = CIParser;

‎lib/links.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,21 @@ class LinkParser {
7575
}
7676
};
7777

78+
const GITHUB_PULL_REQUEST_URL = /github.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/;
79+
80+
LinkParser.parsePRFromURL = function(url) {
81+
if (typeof url !== 'string') {
82+
return undefined;
83+
}
84+
const match = url.match(GITHUB_PULL_REQUEST_URL);
85+
if (match) {
86+
return {
87+
owner: match[1],
88+
repo: match[2],
89+
prid: parseInt(match[3])
90+
};
91+
}
92+
return undefined;
93+
};
94+
7895
module.exports = LinkParser;

‎lib/pr_checker.js‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,8 @@ const {
1717
CONFLICTING
1818
} = require('./mergeable_state');
1919

20-
const CIParser = require('./ci');
21-
const CI_TYPES = CIParser.TYPES;
22-
const { FULL, LITE } = CIParser.constants;
20+
const { JobParser, CI_TYPES, constants } = require('./ci');
21+
const { COMMIT, PR, LITE_PR, LITE_COMMIT } = constants;
2322

2423
class PRChecker {
2524
/**
@@ -187,6 +186,11 @@ class PRChecker {
187186
return true;
188187
}
189188

189+
hasFullOrLiteCI(ciMap) {
190+
return (ciMap.has(COMMIT) || ciMap.has(PR) ||
191+
ciMap.has(LITE_PR) || ciMap.has(LITE_COMMIT));
192+
}
193+
190194
// TODO: we might want to check CI status when it's less flaky...
191195
// TODO: not all PR requires CI...labels?
192196
checkCI() {
@@ -197,13 +201,13 @@ class PRChecker {
197201
};
198202
const { maxCommits } = argv;
199203
const thread = comments.concat([prNode]).concat(reviews);
200-
const ciMap = new CIParser(thread).parse();
204+
const ciMap = new JobParser(thread).parse();
201205
let status = true;
202206
if (!ciMap.size) {
203207
cli.error('No CI runs detected');
204208
this.CIStatus = false;
205209
return false;
206-
} else if (!ciMap.get(FULL) && !ciMap.get(LITE)) {
210+
} else if (!this.hasFullOrLiteCI(ciMap)) {
207211
status = false;
208212
cli.error('No full CI runs or lite CI runs detected');
209213
}

‎test/fixtures/comments_with_ci.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
[
22
{
33
"publishedAt": "2017-10-27T04:16:36.458Z",
4-
"bodyText": "CI: https://ci.nodejs.org/job/node-test-pull-request/10984/\ncitgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1030/"
4+
"bodyText": "CI: https://ci.nodejs.org/job/node-test-pull-request/10984/\ncitgm: https://ci.nodejs.org/job/citgm-smoker/1030/"
55
},
66
{
77
"publishedAt": "2017-10-24T04:16:36.458Z",
8-
"bodyText": "https://ci.nodejs.org/view/libuv/job/libuv-test-commit/537/"
8+
"bodyText": "https://ci.nodejs.org/job/libuv-test-commit/537/"
99
},
1010
{
1111
"publishedAt": "2017-10-23T04:16:36.458Z",
12-
"bodyText": "https://ci.nodejs.org/job/node-test-commit-linux-nointl/7"
12+
"bodyText": "https://ci.nodejs.org/job/node-test-commit-nointl/7/"
1313
},
1414
{
1515
"publishedAt": "2017-10-22T04:16:36.458Z",

0 commit comments

Comments
 (0)