From 55e4f430a4b10b40804d8a1f91d8a2913563ef6f Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 24 Apr 2024 11:52:29 +0200 Subject: [PATCH 1/3] feat(ncu-ci): require `--certify-safe` flag Or ensure the PR has received at least one approving review since last time it was pushed. --- bin/ncu-ci.js | 7 ++++++- lib/ci/run_ci.js | 15 +++++++++++++-- lib/pr_checker.js | 1 + test/unit/ci_start.test.js | 8 ++++---- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/bin/ncu-ci.js b/bin/ncu-ci.js index a84b964b..5bc2023f 100755 --- a/bin/ncu-ci.js +++ b/bin/ncu-ci.js @@ -113,6 +113,11 @@ const args = yargs(hideBin(process.argv)) describe: 'ID of the PR', type: 'number' }) + .positional('certify-safe', { + describe: 'If not provided, the command will reject PRs that have ' + + 'been pushed since the last review', + type: 'boolean' + }) .option('owner', { default: '', describe: 'GitHub repository owner' @@ -291,7 +296,7 @@ class RunPRJobCommand { this.cli.setExitCode(1); return; } - const jobRunner = new RunPRJob(cli, request, owner, repo, prid); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, this.argv.certifySafe); if (!(await jobRunner.start())) { this.cli.setExitCode(1); process.exitCode = 1; diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js index 46891c21..e14ddb41 100644 --- a/lib/ci/run_ci.js +++ b/lib/ci/run_ci.js @@ -7,6 +7,7 @@ import { } from './ci_type_parser.js'; import PRData from '../pr_data.js'; import { debuglog } from '../verbosity.js'; +import PRChecker from '../pr_checker.js'; export const CI_CRUMB_URL = `https://${CI_DOMAIN}/crumbIssuer/api/json`; const CI_PR_NAME = CI_TYPES.get(CI_TYPES_KEYS.PR).jobName; @@ -16,13 +17,16 @@ const CI_V8_NAME = CI_TYPES.get(CI_TYPES_KEYS.V8).jobName; export const CI_V8_URL = `https://${CI_DOMAIN}/job/${CI_V8_NAME}/build`; export class RunPRJob { - constructor(cli, request, owner, repo, prid) { + constructor(cli, request, owner, repo, prid, certifySafe) { this.cli = cli; this.request = request; this.owner = owner; this.repo = repo; this.prid = prid; this.prData = new PRData({ prid, owner, repo }, cli, request); + this.certifySafe = + certifySafe || + new PRChecker(cli, this.prData, request, {}).checkCommitsAfterReview(); } async getCrumb() { @@ -62,7 +66,13 @@ export class RunPRJob { } async start() { - const { cli } = this; + const { cli, certifySafe } = this; + + if (!certifySafe) { + cli.error('Refusing to run CI on potentially unsafe PR'); + return false; + } + cli.startSpinner('Validating Jenkins credentials'); const crumb = await this.getCrumb(); @@ -75,6 +85,7 @@ export class RunPRJob { try { cli.startSpinner('Starting PR CI job'); + throw 'NOOO' const response = await this.request.fetch(CI_PR_URL, { method: 'POST', headers: { diff --git a/lib/pr_checker.js b/lib/pr_checker.js index f4e2b0f7..37cdbd51 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -534,6 +534,7 @@ export default class PRChecker { ); if (reviewIndex === -1) { + cli.warn('No approving reviews found'); return false; } diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index 4c8eea9f..f2285d7d 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -51,7 +51,7 @@ describe('Jenkins', () => { .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); assert.strictEqual(await jobRunner.start(), false); }); @@ -61,7 +61,7 @@ describe('Jenkins', () => { json: sinon.stub().throws() }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); assert.strictEqual(await jobRunner.start(), false); }); @@ -89,7 +89,7 @@ describe('Jenkins', () => { json: sinon.stub().withArgs(CI_CRUMB_URL) .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); assert.ok(await jobRunner.start()); }); @@ -108,7 +108,7 @@ describe('Jenkins', () => { json: sinon.stub().withArgs(CI_CRUMB_URL) .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); assert.strictEqual(await jobRunner.start(), false); }); }); From a2a646534637a653082ba40c9f262628d719b546 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 24 Apr 2024 15:25:19 +0200 Subject: [PATCH 2/3] add test --- lib/ci/run_ci.js | 1 - test/unit/ci_start.test.js | 44 +++++++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js index e14ddb41..29725da8 100644 --- a/lib/ci/run_ci.js +++ b/lib/ci/run_ci.js @@ -85,7 +85,6 @@ export class RunPRJob { try { cli.startSpinner('Starting PR CI job'); - throw 'NOOO' const response = await this.request.fetch(CI_PR_URL, { method: 'POST', headers: { diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index f2285d7d..2628d796 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -1,4 +1,4 @@ -import { describe, it, before } from 'node:test'; +import { describe, it, before, afterEach } from 'node:test'; import assert from 'assert'; import sinon from 'sinon'; @@ -9,6 +9,7 @@ import { CI_CRUMB_URL, CI_PR_URL } from '../../lib/ci/run_ci.js'; +import PRChecker from '../../lib/pr_checker.js'; import TestCLI from '../fixtures/test_cli.js'; @@ -111,4 +112,45 @@ describe('Jenkins', () => { const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); assert.strictEqual(await jobRunner.start(), false); }); + + describe('without --certify-safe flag', () => { + afterEach(() => { + sinon.restore(); + }); + for (const certifySafe of [false, true]) { + it(`should return ${certifySafe} if PR checker reports it as ${ + certifySafe ? '' : 'potentially un' + }safe`, async() => { + const cli = new TestCLI(); + + sinon.replace(PRChecker.prototype, 'checkCommitsAfterReview', + sinon.fake.returns(certifySafe)); + + const request = { + gql: sinon.stub().returns({ + repository: { + pullRequest: { + labels: { + nodes: [] + } + } + } + }), + fetch: sinon.stub() + .callsFake((url, { method, headers, body }) => { + assert.strictEqual(url, CI_PR_URL); + assert.strictEqual(method, 'POST'); + assert.deepStrictEqual(headers, { 'Jenkins-Crumb': crumb }); + assert.ok(body._validated); + return Promise.resolve({ status: 201 }); + }), + json: sinon.stub().withArgs(CI_CRUMB_URL) + .returns(Promise.resolve({ crumb })) + }; + + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, false); + assert.strictEqual(await jobRunner.start(), certifySafe); + }); + } + }); }); From cd5ca9a756983b3a2517f8b18765c8d9010f440d Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 24 Apr 2024 18:23:03 +0200 Subject: [PATCH 3/3] fix tests --- test/unit/ci_start.test.js | 4 ++-- test/unit/pr_checker.test.js | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index 2628d796..0058b1e0 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -113,11 +113,11 @@ describe('Jenkins', () => { assert.strictEqual(await jobRunner.start(), false); }); - describe('without --certify-safe flag', () => { + describe('without --certify-safe flag', { concurrency: false }, () => { afterEach(() => { sinon.restore(); }); - for (const certifySafe of [false, true]) { + for (const certifySafe of [true, false]) { it(`should return ${certifySafe} if PR checker reports it as ${ certifySafe ? '' : 'potentially un' }safe`, async() => { diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index a86a837d..3e10a71c 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -2179,7 +2179,9 @@ describe('PRChecker', () => { it('should skip the check if there are no reviews', () => { const { commits } = multipleCommitsAfterReview; - const expectedLogs = {}; + const expectedLogs = { + warn: [['No approving reviews found']] + }; const data = { pr: firstTimerPR,