From a5213cde57b62ad5c6abb948415e3f110beaf929 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 24 Apr 2024 20:45:49 +0200 Subject: [PATCH] feat(ncu-ci)!: require `--certify-safe` flag (#798) 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 | 14 ++++++++-- lib/pr_checker.js | 1 + test/unit/ci_start.test.js | 52 ++++++++++++++++++++++++++++++++---- test/unit/pr_checker.test.js | 4 ++- 5 files changed, 69 insertions(+), 9 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..29725da8 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(); 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..0058b1e0 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'; @@ -51,7 +52,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 +62,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 +90,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 +109,48 @@ 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); }); + + describe('without --certify-safe flag', { concurrency: false }, () => { + afterEach(() => { + sinon.restore(); + }); + for (const certifySafe of [true, false]) { + 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); + }); + } + }); }); 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,