diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js index 1561030b..e8e00b4a 100644 --- a/lib/ci/run_ci.js +++ b/lib/ci/run_ci.js @@ -16,6 +16,26 @@ export const CI_PR_URL = `https://${CI_DOMAIN}/job/${CI_PR_NAME}/build`; 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`; +const REQUEST_CI_COMMENT = /^@nodejs-github-bot .+([0-9a-f]{40})\.*\n*$/; + +async function findSafeFullCommitSHA(cli, prData, request) { + // First, let's check if the head was approved. + await Promise.all([prData.getReviews(), prData.getCommits()]); + + const approvedHead = new PRChecker(cli, prData, request, {}).getApprovedTipOfHead(); + if (approvedHead) return approvedHead; + + // Else, let's find out if a collaborator added a comment with a full commit SHA. + await Promise.all([prData.getCollaborators(), prData.getComments()]); + const collaborators = Array.from(prData.collaborators.values(), + (c) => c.login.toLowerCase()); + return prData.comments + .findLast(c => + collaborators.includes(c.author.login.toLowerCase()) && + REQUEST_CI_COMMENT.test(c.body)) + ?.body.match(REQUEST_CI_COMMENT)[1]; +} + export class RunPRJob { constructor(cli, request, owner, repo, prid, certifySafe) { this.cli = cli; @@ -26,9 +46,7 @@ export class RunPRJob { this.prData = new PRData({ prid, owner, repo }, cli, request); this.certifySafe = certifySafe || - Promise.all([this.prData.getReviews(), this.prData.getCommits()]).then(() => - (this.certifySafe = new PRChecker(cli, this.prData, request, {}).getApprovedTipOfHead()) - ); + findSafeFullCommitSHA(cli, this.prData, request).then((h) => (this.certifySafe = h)); } async getCrumb() { @@ -72,6 +90,8 @@ export class RunPRJob { const { cli, certifySafe } = this; if (!(await certifySafe)) { + cli.info('If the tip of this PR looks safe, add a comment in the form of:'); + cli.info('@nodejs-github-bot test '); cli.error('Refusing to run CI on potentially unsafe PR'); return false; } diff --git a/lib/queries/PRComments.gql b/lib/queries/PRComments.gql index 5234dc44..faafa9d1 100644 --- a/lib/queries/PRComments.gql +++ b/lib/queries/PRComments.gql @@ -9,6 +9,7 @@ query Comments($prid: Int!, $owner: String!, $repo: String!, $after: String) { } nodes { publishedAt + body bodyText author { login diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index c56e5308..e03b4fbe 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -11,6 +11,7 @@ import { CI_PR_URL } from '../../lib/ci/run_ci.js'; import PRChecker from '../../lib/pr_checker.js'; +import PRData from '../../lib/pr_data.js'; import TestCLI from '../fixtures/test_cli.js'; @@ -19,14 +20,15 @@ describe('Jenkins', () => { const repo = 'node-auto-test'; const prid = 123456; const crumb = 'asdf1234'; + const dummySHA = '51ce389dc1d539216d30bba0986a8c270801d65f'; before(() => { - sinon.stub(FormData.prototype, 'append').callsFake(function(key, value) { + const stubbed = sinon.stub(FormData.prototype, 'append').callsFake(function(key, value) { assert.strictEqual(key, 'json'); const { parameter } = JSON.parse(value); const expectedParameters = { CERTIFY_SAFE: 'on', - COMMIT_SHA_CHECK: 'deadbeef', + COMMIT_SHA_CHECK: dummySHA, TARGET_GITHUB_ORG: owner, TARGET_REPO_NAME: repo, PR_ID: prid, @@ -41,8 +43,10 @@ describe('Jenkins', () => { this._validated = true; - return FormData.prototype.append.wrappedMethod.bind(this)(key, value); + return Reflect.apply(FormData.prototype.append.wrappedMethod, this, arguments); }); + + return () => stubbed.restore(); }); it('should fail if starting node-pull-request throws', async() => { @@ -54,7 +58,7 @@ describe('Jenkins', () => { .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, dummySHA); assert.strictEqual(await jobRunner.start(), false); }); @@ -64,7 +68,7 @@ describe('Jenkins', () => { json: sinon.stub().throws() }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, dummySHA); assert.strictEqual(await jobRunner.start(), false); }); @@ -92,7 +96,7 @@ describe('Jenkins', () => { json: sinon.stub().withArgs(CI_CRUMB_URL) .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, 'deadbeef'); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, dummySHA); assert.ok(await jobRunner.start()); }); @@ -111,22 +115,61 @@ describe('Jenkins', () => { json: sinon.stub().withArgs(CI_CRUMB_URL) .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, dummySHA); assert.strictEqual(await jobRunner.start(), false); }); describe('without --certify-safe flag', { concurrency: false }, () => { + before(() => { + sinon.replace(PRData.prototype, 'getReviews', function() {}); + sinon.replace(PRData.prototype, 'getCommits', function() {}); + return () => { + PRData.prototype.getReviews.restore(); + PRData.prototype.getCommits.restore(); + }; + }); afterEach(() => { - sinon.restore(); + PRData.prototype.getCollaborators.restore(); + PRData.prototype.getComments.restore(); + PRChecker.prototype.getApprovedTipOfHead.restore(); }); - for (const certifySafe of [true, false]) { - it(`should return ${certifySafe} if PR checker reports it as ${ - certifySafe ? '' : 'potentially un' - }safe`, async() => { + + for (const { headIsApproved = false, collaborators = [], comments = [], expected } of [{ + headIsApproved: true, + expected: true, + }, { + headIsApproved: false, + expected: false, + }, { + collaborators: ['foo'], + comments: [{ login: 'foo' }], + expected: true, + }, { + // Validates that passing full commit URL also works. + collaborators: ['foo'], + comments: [{ login: 'foo', body: `@nodejs-github-bot test https://github.com/nodejs/node/commit/${dummySHA}.\n` }], + expected: true, + }, { + // Validates that non-collaborator commenting should have no effect. + collaborators: ['foo'], + comments: [{ login: 'bar' }], + expected: false, + }]) { + it(`should return ${expected} with ${ + JSON.stringify({ headIsApproved, collaborators, comments })}`, async() => { const cli = new TestCLI(); - sinon.replace(PRChecker.prototype, 'getApprovedTipOfHead', - sinon.fake.returns(certifySafe && 'deadbeef')); + sinon.stub(PRData.prototype, 'getCollaborators').callsFake(function() { + this.collaborators = collaborators.map(login => ({ login })); + }); + sinon.stub(PRData.prototype, 'getComments').callsFake(function() { + this.comments = comments.map(({ body, login }) => ({ + body: body ?? `@nodejs-github-bot test ${dummySHA}`, + author: { login } + })); + }); + sinon.stub(PRChecker.prototype, 'getApprovedTipOfHead').callsFake( + sinon.fake.returns(headIsApproved && dummySHA)); const request = { gql: sinon.stub().returns({ @@ -151,7 +194,7 @@ describe('Jenkins', () => { }; const jobRunner = new RunPRJob(cli, request, owner, repo, prid, false); - assert.strictEqual(await jobRunner.start(), certifySafe); + assert.strictEqual(await jobRunner.start(), expected); }); } });